Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 0/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 v1: https://listman.redhat.com/archives/libguestfs/2022-September/029806.html Please see the Notes section on each patch for the updates in this version (addressing v1 feedback). Patch #5 is a candidate for dropping in particular. I'm also including a range-diff between v1 and v2, below.> 1: 4ec0fa83a1a2 ! 1: bdbd76659e43 gui: check VCPU & memory limits upon showing the conversion dialog > @@ -17,6 +17,9 @@ > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > + Acked-by: Richard W.M. Jones <rjones at redhat.com> > + v2: > + - pick up Rich's ACK > > diff --git a/gui.c b/gui.c > --- a/gui.c > 2: 9f7005fecaf4 ! 2: 0924f02a260b restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1 > @@ -113,6 +113,16 @@ > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > + Reviewed-by: Richard W.M. Jones <rjones at redhat.com> > + v2: > + > + - pick up Rich's R-b > + > + - rename dense_topo to phys_topo [Rich] > + > + - take Rich's suggested wording for the manual > + > + - rewrap the additions to the manual > > diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl > --- a/generate-p2v-config.pl > @@ -125,7 +135,7 @@ > + ConfigSection->new( > + name => 'vcpu', > + elements => [ > -+ ConfigBool->new(name => 'dense_topo'), > ++ ConfigBool->new(name => 'phys_topo'), > + ConfigInt->new(name => 'cores', value => 0), > + ], > + ), > @@ -146,23 +156,23 @@ > use a randomly generated name.", > ), > - "p2v.vcpus" => manual_entry->new( > -+ "p2v.vcpu.dense_topo" => manual_entry->new( > ++ "p2v.vcpu.phys_topo" => manual_entry->new( > + shortopt => "", # ignored for booleans > + description => " > -+Copy the physical machine's CPU topology, densely populated, to the > -+guest. Disabled by default. If disabled, the C<p2v.vcpu.cores> setting > -+takes effect.", > ++Copy the physical machine's complete CPU topology (sockets, cores and > ++threads) to the guest. Disabled by default. If disabled, the > ++C<p2v.vcpu.cores> setting takes effect.", > + ), > + "p2v.vcpu.cores" => manual_entry->new( > shortopt => "N", > description => " > -The number of virtual CPUs to give to the guest. The default is to > -use the same as the number of physical CPUs.", > -+This setting is ignored if C<p2v.vcpu.dense_topo> is enabled. > -+Otherwise, it specifies the flat number of vCPU cores to give to the > -+guest (placing all of those cores into a single socket, and exposing one > -+thread per core). The default value is the number of online logical > -+processors on the physical machine.", > ++This setting is ignored if C<p2v.vcpu.phys_topo> is enabled. Otherwise, > ++it specifies the flat number of vCPU cores to give to the guest (placing > ++all of those cores into a single socket, and exposing one thread per > ++core). The default value is the number of online logical processors on > ++the physical machine.", > ), > "p2v.memory" => manual_entry->new( > shortopt => "n(M|G)", > @@ -297,7 +307,7 @@ > } > config->guestname = strdup (hostname); > > -+ config->vcpu.dense_topo = false; > ++ config->vcpu.phys_topo = false; > + > /* Defaults for #vcpus and memory are taken from the physical machine. */ > i = sysconf (_SC_NPROCESSORS_ONLN); > @@ -357,7 +367,7 @@ > - } end_element (); > - } > - } end_element (); > -+ if (config->vcpu.dense_topo) > ++ if (config->vcpu.phys_topo) > + get_cpu_topology (&topo); > + else { > + topo.sockets = 1; > @@ -407,7 +417,7 @@ > grep "^auth\.sudo.*false" $out > grep "^guestname.*test" $out > -grep "^vcpus.*4" $out > -+grep "^vcpu.dense_topo.*false" $out > ++grep "^vcpu.phys_topo.*false" $out > +grep "^vcpu.cores.*4" $out > grep "^memory.*"$((1024*1024*1024)) $out > grep "^disks.*sda sdb sdc" $out > 3: 3e873b661eed ! 3: 8c37949d1ea2 gui: set row count from a running variable when populating tables > @@ -14,8 +14,8 @@ > > (This patch is easiest to review with "git show --word-diff=color".) > > - Don't do the same simplification for colums, as we're going to introduce a > - multi-column widget next. > + Don't do the same simplification for columns, as we're going to introduce > + a multi-column widget next. > > Note that one definition of table_attach() now evaluates "top" twice. > Preventing that would be a mess: we could be tempted to introduce a do { > @@ -28,6 +28,12 @@ > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > + Reviewed-by: Richard W.M. Jones <rjones at redhat.com> > + v2: > + > + - s/colums/columns/ in the commit message [Rich] > + > + - pick up Rich's R-b > > diff --git a/gui-gtk3-compat.h b/gui-gtk3-compat.h > --- a/gui-gtk3-compat.h > 4: 1fcf2898202f ! 4: 6e3a17d86f89 gui: offer copying the vCPU topology from the fully populated physical one > @@ -25,6 +25,12 @@ > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > + Reviewed-by: Richard W.M. Jones <rjones at redhat.com> > + v2: > + > + - pick up Rich's R-b > + > + - s/dense_topo/phys_topo/ everywhere [Rich] > > diff --git a/gui.c b/gui.c > --- a/gui.c > @@ -51,7 +57,7 @@ > +static void vcpu_topo_toggled (GtkWidget *w, gpointer data); > static void vcpus_or_memory_check_callback (GtkWidget *w, gpointer data); > static void notify_ui_callback (int type, const char *data); > -+static bool get_dense_topo_from_conv_dlg (void); > ++static bool get_phys_topo_from_conv_dlg (void); > static int get_vcpus_from_conv_dlg (void); > static uint64_t get_memory_from_conv_dlg (void); > > @@ -72,7 +78,7 @@ > + vcpu_topo = gtk_check_button_new_with_mnemonic ( > + _("Copy fully populated _pCPU topology")); > + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (vcpu_topo), > -+ config->vcpu.dense_topo); > ++ config->vcpu.phys_topo); > + table_attach (target_tbl, vcpu_topo, 0, 2, row, GTK_FILL, GTK_FILL, 1, 1); > + > row++; > @@ -110,12 +116,12 @@ > +static void > +vcpu_topo_toggled (GtkWidget *w, gpointer data) > +{ > -+ bool dense_topo; > ++ bool phys_topo; > + unsigned vcpus; > + char vcpus_str[64]; > + > -+ dense_topo = get_dense_topo_from_conv_dlg (); > -+ if (dense_topo) { > ++ phys_topo = get_phys_topo_from_conv_dlg (); > ++ if (phys_topo) { > + struct cpu_topo topo; > + > + get_cpu_topology (&topo); > @@ -126,7 +132,7 @@ > + > + snprintf (vcpus_str, sizeof vcpus_str, "%u", vcpus); > + gtk_entry_set_text (GTK_ENTRY (vcpus_entry), vcpus_str); > -+ gtk_widget_set_sensitive (vcpus_entry, !dense_topo); > ++ gtk_widget_set_sensitive (vcpus_entry, !phys_topo); > +} > + > /** > @@ -137,7 +143,7 @@ > } > > +static bool > -+get_dense_topo_from_conv_dlg (void) > ++get_phys_topo_from_conv_dlg (void) > +{ > + return gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (vcpu_topo)); > +} > @@ -149,7 +155,7 @@ > return; > } > > -+ config->vcpu.dense_topo = get_dense_topo_from_conv_dlg (); > ++ config->vcpu.phys_topo = get_phys_topo_from_conv_dlg (); > config->vcpu.cores = get_vcpus_from_conv_dlg (); > config->memory = get_memory_from_conv_dlg (); > > 5: a5ce2ce0843c ! 5: 3e87bcfec5ab copy fully populated vCPU topology by default > @@ -6,6 +6,15 @@ > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > + Acked-by: Richard W.M. Jones <rjones at redhat.com> > + v2: > + > + - pick up Rich's A-b > + > + - resolve rebase conflicts due to dense_topo -> phys_topo renaming > + > + - we can drop this patch, per Daniel's comment > + <https://listman.redhat.com/archives/libguestfs/2022-September/029841.html> > > diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl > --- a/generate-p2v-config.pl > @@ -13,10 +22,10 @@ > @@ > shortopt => "", # ignored for booleans > description => " > - Copy the physical machine's CPU topology, densely populated, to the > --guest. Disabled by default. If disabled, the C<p2v.vcpu.cores> setting > -+guest. This is the default. If disabled, the C<p2v.vcpu.cores> setting > - takes effect.", > + Copy the physical machine's complete CPU topology (sockets, cores and > +-threads) to the guest. Disabled by default. If disabled, the > ++threads) to the guest. This is the default. If disabled, the > + C<p2v.vcpu.cores> setting takes effect.", > ), > "p2v.vcpu.cores" => manual_entry->new( > > @@ -27,8 +36,8 @@ > } > config->guestname = strdup (hostname); > > -- config->vcpu.dense_topo = false; > -+ config->vcpu.dense_topo = true; > +- config->vcpu.phys_topo = false; > ++ config->vcpu.phys_topo = true; > > /* Defaults for #vcpus and memory are taken from the physical machine. */ > i = sysconf (_SC_NPROCESSORS_ONLN); > @@ -41,7 +50,7 @@ > > # The Linux kernel command line. > -$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpu.cores=4 p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1 p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out > -+$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpu.dense_topo=false p2v.vcpu.cores=4 p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1 p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out > ++$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpu.phys_topo=false p2v.vcpu.cores=4 p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1 p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out > > # For debugging purposes. > cat $out > 6: d9d09c8ad7e6 ! 6: 1ecbd348a4b3 Makefile.am: set vCPU topology to 1*2*2 in the "p2v in a VM" tests > @@ -2,11 +2,17 @@ > > Makefile.am: set vCPU topology to 1*2*2 in the "p2v in a VM" tests > > - This lets us exercise both states of the "p2v.vcpu.dense_topo" switch > + This lets us exercise both states of the "p2v.vcpu.phys_topo" switch > sensibly via the in-VM GUI. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > + Acked-by: Richard W.M. Jones <rjones at redhat.com> > + v2: > + > + - pick up Rich's A-b > + > + - s/dense_topo/phys_topo/ in the commit message [Rich] > > diff --git a/Makefile.am b/Makefile.am > --- a/Makefile.amThanks, Laszlo Laszlo Ersek (6): gui: check VCPU & memory limits upon showing the conversion dialog restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1 gui: set row count from a running variable when populating tables gui: offer copying the vCPU topology from the fully populated physical one copy fully populated vCPU topology by default Makefile.am: set vCPU topology to 1*2*2 in the "p2v in a VM" tests Makefile.am | 2 + generate-p2v-config.pl | 45 ++++--- gui-gtk3-compat.h | 8 +- p2v.h | 6 + cpuid.c | 39 ++++--- gui.c | 123 ++++++++++++++------ main.c | 8 +- physical-xml.c | 54 ++++----- test-virt-p2v-cmdline.sh | 5 +- 9 files changed, 182 insertions(+), 108 deletions(-)
Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 1/6] gui: check VCPU & memory limits upon showing the conversion dialog
show_conversion_dialog() hides the VCPU and memory limit warning signs
unconditionally. The warning signs and the associated label text(s) can
only be displayed later, when the VCPUs or the memory entry changes, and
vcpus_or_memory_check_callback() is called.
This is incorrect: we may initialize each of these entries from the
physical machine such that it breaks the corresponding limit at once. (The
problem can be triggered by reducing MAX_SUPPORTED_VCPUS to (say) 1, and
invoking "make run-virt-p2v-directly" on a dual-core (at least)
computer.)
Therefore call vcpus_or_memory_check_callback() from
show_conversion_dialog() immediately, in place of hiding the warning signs
unconditionally.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Acked-by: Richard W.M. Jones <rjones at redhat.com>
---
Notes:
v2:
- pick up Rich's ACK
gui.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gui.c b/gui.c
index 227e41d2516f..5c4f1343095a 100644
--- a/gui.c
+++ b/gui.c
@@ -973,8 +973,7 @@ show_conversion_dialog (void)
/* Show the conversion dialog. */
gtk_widget_show_all (conv_dlg);
- gtk_widget_hide (vcpus_warning);
- gtk_widget_hide (memory_warning);
+ vcpus_or_memory_check_callback (NULL, NULL);
/* output_drivers may have been updated, so repopulate o_combo. */
repopulate_output_combo (NULL);
Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1
Currently, the CPU topology for the converted domain is determined as
follows:
(1) main() [main.c]
(2) set_config_defaults() [main.c]
vcpus <-- sysconf (_SC_NPROCESSORS_ONLN)
get_cpu_config() [cpuid.c]
get_topology() [cpuid.c]
sockets <-- lscpu (physical)
cores <-- lscpu (physical)
threads <-- lscpu (physical)
(3) update_config_from_kernel_cmdline() [kernel-config.c]
vcpus <-- kernel cmdline
sockets <-- kernel cmdline
cores <-- kernel cmdline
threads <-- kernel cmdline
(4) opt#1: kernel_conversion() [kernel.c]
no change to topology info
opt#2: gui_conversion() [gui.c]
vcpus <-- GUI
(5) ...
start_conversion() [conversion.c]
generate_physical_xml() [physical-xml.c]
format vcpus, sockets, cores, threads
In (2), we initialize the topology information from the physical machine.
In (3), we override each property in isolation from the kernel command
line (this is done for both kernel conversion and GUI conversion; in the
former case, it is the only way for the user to influence each element of
the topology). In (4), in case we picked GUI conversion, the VCPU number
can be overridden yet another time on the GUI. In (5), we format the
topology information into the domain XML.
The problem is that it's easy to create inconsistent topologies that
libvirt complains about. One example is that in step (4) on the GUI, if
the flat VCPU count is reduced by the user, it can result in sockets
partially populated with cores or threads, or in cores partially populated
with threads. Another example (even if the user does not touch the VCPU
count) is a partially onlined CPU topology on the physical machine:
_SC_NPROCESSORS_ONLN does not reflect any topology information, and if the
flat number it returns is less than the (sockets * cores * threads)
product from "lscpu", then the online logical processors will be
"compressed to the left" by libvirt just the same as after the manual
VCPU
count reduction.
An over-complicated way to fix this would be the following:
- parse the output of "lscpu -p" (which is available since RHEL6 --
"lscpu" is altogether missing in RHEL5), retrieving online-ness
combined
with full topology information
- expose complete topology info on the GUI
- format the complete topology information for libvirt.
A simpler way (which is what this patch series chooses) is to remove some
flexibility from virt-p2v's configuration interface. Namely, only allow
the user to choose betwen (a) copying the physical CPU topology, *fully
populated*, (b) specifying the core count N for a "1 socket * N
cores/socket * 1 thread/core" topology.
Option (a) eliminates the "partially onlined physical CPU topology"
scenario; it produces a topology that exists in the physical world, and
does not require p2v to maintain topology information more expressively
than it currently does.
Option (b) eliminates any conflicts between the physical (or any
user-configured) sockets:cores:threads hierarchy and a manually set
logical processor count. Option (b) produces the kind of flat CPU topology
that QEMU has defaulted to since version 6.2, with a simple "-smp"
option.
Option (b) preserves the simple p2v user interface for entering only a
logical processor count.
In this patch:
- delete the (sockets, cores, threads) triplet from the p2v config
structure, moving it out to a stand-alone structure,
- introduce a new boolean config element (taking priority above the
existent flat VCPU count) for copying -- densely populated -- the
physical CPU topology,
- always render the full topology into the domain XML, handling both
options (a) and (b) -- for option (a), invoke "lscpu" right when the
physical topology is needed,
- make option (b) the default (sticking with current practice),
- do not expose option (a) on the GUI yet.
(An interesting question is whether post-patch, in GUI mode, where the
conversion runs in a separate thread, it is safe to fork a child process
for "lscpu" from said thread, with popen():
start_conversion_thread() -> start_conversion() -> generate_physical_xml()
-> get_cpu_topology() -> get_lscpu() -> popen().
This concern arises because in the child process, only the one thread
returning from fork() exists, thus if any locks are held by other threads
in the parent process, those will never be released in the child process.
But such a fork() should be safe: existent code already starts e.g. nbdkit
on the same call path, namely start_conversion_thread() ->
start_conversion() -> start_nbd_server() -> start_nbdkit() -> fork().)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---
Notes:
v2:
- pick up Rich's R-b
- rename dense_topo to phys_topo [Rich]
- take Rich's suggested wording for the manual
- rewrap the additions to the manual
generate-p2v-config.pl | 45 ++++++++--------
p2v.h | 6 +++
cpuid.c | 39 ++++++++------
gui.c | 4 +-
main.c | 8 +--
physical-xml.c | 54 ++++++++++----------
test-virt-p2v-cmdline.sh | 5 +-
7 files changed, 88 insertions(+), 73 deletions(-)
diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl
index 06a2e2c8b0e4..47487f7b0f35 100755
--- a/generate-p2v-config.pl
+++ b/generate-p2v-config.pl
@@ -110,16 +110,19 @@ my @fields = [
],
),
ConfigString->new(name => 'guestname'),
- ConfigInt->new(name => 'vcpus', value => 0),
+ ConfigSection->new(
+ name => 'vcpu',
+ elements => [
+ ConfigBool->new(name => 'phys_topo'),
+ ConfigInt->new(name => 'cores', value => 0),
+ ],
+ ),
ConfigUInt64->new(name => 'memory'),
ConfigSection->new(
name => 'cpu',
elements => [
ConfigString->new(name => 'vendor'),
ConfigString->new(name => 'model'),
- ConfigUnsigned->new(name => 'sockets'),
- ConfigUnsigned->new(name => 'cores'),
- ConfigUnsigned->new(name => 'threads'),
ConfigBool->new(name => 'acpi'),
ConfigBool->new(name => 'apic'),
ConfigBool->new(name => 'pae'),
@@ -231,11 +234,21 @@ The name of the guest that is created. The default is to
try to
derive a name from the physical machine?s hostname (if possible) else
use a randomly generated name.",
),
- "p2v.vcpus" => manual_entry->new(
+ "p2v.vcpu.phys_topo" => manual_entry->new(
+ shortopt => "", # ignored for booleans
+ description => "
+Copy the physical machine's complete CPU topology (sockets, cores and
+threads) to the guest. Disabled by default. If disabled, the
+C<p2v.vcpu.cores> setting takes effect.",
+ ),
+ "p2v.vcpu.cores" => manual_entry->new(
shortopt => "N",
description => "
-The number of virtual CPUs to give to the guest. The default is to
-use the same as the number of physical CPUs.",
+This setting is ignored if C<p2v.vcpu.phys_topo> is enabled. Otherwise,
+it specifies the flat number of vCPU cores to give to the guest (placing
+all of those cores into a single socket, and exposing one thread per
+core). The default value is the number of online logical processors on
+the physical machine.",
),
"p2v.memory" => manual_entry->new(
shortopt => "n(M|G)",
@@ -258,24 +271,6 @@ the same CPU vendor as the physical machine.",
description => "
The vCPU model, eg. \"IvyBridge\". The default is to use the same
CPU model as the physical machine.",
- ),
- "p2v.cpu.sockets" => manual_entry->new(
- shortopt => "N",
- description => "
-Number of vCPU sockets to use. The default is to use the same as the
-physical machine.",
- ),
- "p2v.cpu.cores" => manual_entry->new(
- shortopt => "N",
- description => "
-Number of vCPU cores to use. The default is to use the same as the
-physical machine.",
- ),
- "p2v.cpu.threads" => manual_entry->new(
- shortopt => "N",
- description => "
-Number of vCPU hyperthreads to use. The default is to use the same
-as the physical machine.",
),
"p2v.cpu.acpi" => manual_entry->new(
shortopt => "", # ignored for booleans
diff --git a/p2v.h b/p2v.h
index 20c4ac7e506a..32dc55d94de5 100644
--- a/p2v.h
+++ b/p2v.h
@@ -60,6 +60,12 @@ extern int feature_colours_option;
extern int force_colour;
/* cpuid.c */
+struct cpu_topo {
+ unsigned sockets;
+ unsigned cores;
+ unsigned threads;
+};
+extern void get_cpu_topology (struct cpu_topo *topo);
extern void get_cpu_config (struct cpu_config *);
/* rtc.c */
diff --git a/cpuid.c b/cpuid.c
index 84603bbb3374..31a1720dc23c 100644
--- a/cpuid.c
+++ b/cpuid.c
@@ -154,22 +154,32 @@ get_vendor (char **lscpu, struct cpu_config *cpu)
}
/**
- * Read the CPU topology from lscpu output.
+ * Read the CPU topology from a separate lscpu invocation.
*/
-static void
-get_topology (char **lscpu, struct cpu_config *cpu)
+void
+get_cpu_topology (struct cpu_topo *topo)
{
- const char *v;
-
- v = get_field (lscpu, "Socket(s)");
- if (v)
- ignore_value (sscanf (v, "%u", &cpu->sockets));
- v = get_field (lscpu, "Core(s) per socket");
- if (v)
- ignore_value (sscanf (v, "%u", &cpu->cores));
- v = get_field (lscpu, "Thread(s) per core");
- if (v)
- ignore_value (sscanf (v, "%u", &cpu->threads));
+
+ CLEANUP_FREE_STRING_LIST char **lscpu = NULL;
+
+ lscpu = get_lscpu ();
+
+ if (lscpu != NULL) {
+ const char *sockets, *cores, *threads;
+
+ sockets = get_field (lscpu, "Socket(s)");
+ cores = get_field (lscpu, "Core(s) per socket");
+ threads = get_field (lscpu, "Thread(s) per core");
+ if (sockets && cores && threads) {
+ ignore_value (sscanf (sockets, "%u", &topo->sockets));
+ ignore_value (sscanf (cores, "%u", &topo->cores));
+ ignore_value (sscanf (threads, "%u", &topo->threads));
+ return;
+ }
+ }
+ topo->sockets = 1;
+ topo->cores = 1;
+ topo->threads = 1;
}
/**
@@ -211,7 +221,6 @@ get_cpu_config (struct cpu_config *cpu)
lscpu = get_lscpu ();
if (lscpu != NULL) {
get_vendor (lscpu, cpu);
- get_topology (lscpu, cpu);
get_flags (lscpu, cpu);
}
diff --git a/gui.c b/gui.c
index 5c4f1343095a..b4a9fed18410 100644
--- a/gui.c
+++ b/gui.c
@@ -767,7 +767,7 @@ create_conversion_dialog (struct config *config)
set_alignment (vcpus_label, 1., 0.5);
vcpus_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (vcpus_label), vcpus_entry);
- snprintf (vcpus_str, sizeof vcpus_str, "%d", config->vcpus);
+ snprintf (vcpus_str, sizeof vcpus_str, "%d",
config->vcpu.cores);
gtk_entry_set_text (GTK_ENTRY (vcpus_entry), vcpus_str);
table_attach (target_tbl, vcpus_entry,
1, 2, 1, 2, GTK_FILL, GTK_FILL, 1, 1);
@@ -2010,7 +2010,7 @@ start_conversion_clicked (GtkWidget *w, gpointer data)
return;
}
- config->vcpus = get_vcpus_from_conv_dlg ();
+ config->vcpu.cores = get_vcpus_from_conv_dlg ();
config->memory = get_memory_from_conv_dlg ();
/* Get the list of disks to be converted. */
diff --git a/main.c b/main.c
index 0ebb7291c7ce..6c44183b65b3 100644
--- a/main.c
+++ b/main.c
@@ -291,16 +291,18 @@ set_config_defaults (struct config *config)
}
config->guestname = strdup (hostname);
+ config->vcpu.phys_topo = false;
+
/* Defaults for #vcpus and memory are taken from the physical machine. */
i = sysconf (_SC_NPROCESSORS_ONLN);
if (i == -1) {
perror ("sysconf: _SC_NPROCESSORS_ONLN");
- config->vcpus = 1;
+ config->vcpu.cores = 1;
}
else if (i == 0)
- config->vcpus = 1;
+ config->vcpu.cores = 1;
else
- config->vcpus = i;
+ config->vcpu.cores = i;
i = sysconf (_SC_PHYS_PAGES);
if (i == -1) {
diff --git a/physical-xml.c b/physical-xml.c
index 4e830ea7ee0c..983798f8a365 100644
--- a/physical-xml.c
+++ b/physical-xml.c
@@ -62,6 +62,7 @@ generate_physical_xml (struct config *config, struct data_conn
*data_conns,
uint64_t memkb;
CLEANUP_XMLFREETEXTWRITER xmlTextWriterPtr xo = NULL;
size_t i;
+ struct cpu_topo topo;
xo = xmlNewTextWriterFilename (filename, 0);
if (xo == NULL)
@@ -104,34 +105,35 @@ generate_physical_xml (struct config *config, struct
data_conn *data_conns,
string_format ("%" PRIu64, memkb);
} end_element ();
- single_element_format ("vcpu", "%d", config->vcpus);
-
- if (config->cpu.vendor || config->cpu.model ||
- config->cpu.sockets || config->cpu.cores ||
config->cpu.threads) {
- /* https://libvirt.org/formatdomain.html#elementsCPU */
- start_element ("cpu") {
- attribute ("match", "minimum");
- if (config->cpu.vendor)
- single_element ("vendor", config->cpu.vendor);
- if (config->cpu.model) {
- start_element ("model") {
- attribute ("fallback", "allow");
- string (config->cpu.model);
- } end_element ();
- }
- if (config->cpu.sockets || config->cpu.cores ||
config->cpu.threads) {
- start_element ("topology") {
- if (config->cpu.sockets)
- attribute_format ("sockets", "%u",
config->cpu.sockets);
- if (config->cpu.cores)
- attribute_format ("cores", "%u",
config->cpu.cores);
- if (config->cpu.threads)
- attribute_format ("threads", "%u",
config->cpu.threads);
- } end_element ();
- }
- } end_element ();
+ if (config->vcpu.phys_topo)
+ get_cpu_topology (&topo);
+ else {
+ topo.sockets = 1;
+ topo.cores = config->vcpu.cores;
+ topo.threads = 1;
}
+ single_element_format ("vcpu", "%u",
+ topo.sockets * topo.cores * topo.threads);
+
+ /* https://libvirt.org/formatdomain.html#elementsCPU */
+ start_element ("cpu") {
+ attribute ("match", "minimum");
+ if (config->cpu.vendor)
+ single_element ("vendor", config->cpu.vendor);
+ if (config->cpu.model) {
+ start_element ("model") {
+ attribute ("fallback", "allow");
+ string (config->cpu.model);
+ } end_element ();
+ }
+ start_element ("topology") {
+ attribute_format ("sockets", "%u", topo.sockets);
+ attribute_format ("cores", "%u", topo.cores);
+ attribute_format ("threads", "%u", topo.threads);
+ } end_element ();
+ } end_element ();
+
switch (config->rtc.basis) {
case BASIS_UNKNOWN:
/* Don't emit any <clock> element. */
diff --git a/test-virt-p2v-cmdline.sh b/test-virt-p2v-cmdline.sh
index cd76044195a3..79c3061b1e7c 100755
--- a/test-virt-p2v-cmdline.sh
+++ b/test-virt-p2v-cmdline.sh
@@ -26,7 +26,7 @@ out=test-virt-p2v-cmdline.out
rm -f $out
# The Linux kernel command line.
-$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user
p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpus=4
p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1
p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp
p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out
+$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user
p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpu.cores=4
p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1
p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp
p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out
# For debugging purposes.
cat $out
@@ -37,7 +37,8 @@ grep "^remote\.port.*123" $out
grep "^auth\.username.*user" $out
grep "^auth\.sudo.*false" $out
grep "^guestname.*test" $out
-grep "^vcpus.*4" $out
+grep "^vcpu.phys_topo.*false" $out
+grep "^vcpu.cores.*4" $out
grep "^memory.*"$((1024*1024*1024)) $out
grep "^disks.*sda sdb sdc" $out
grep "^removable.*sdd" $out
Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 3/6] gui: set row count from a running variable when populating tables
Each call to table_attach() in fact open-codes (row, row + 1), for setting
the top and bottom grid lines of the widget being added to the table.
What's more, within a given table, we add widgets in a left-to-right
first, top-down second fashion, so "row" only ever increases within a
particular table.
Modify the table_attach() macro to calculate "bottom" automatically,
plus
replace the open-coded "row" constants with actual (incremented)
"row"
variables.
(This patch is easiest to review with "git show --word-diff=color".)
Don't do the same simplification for columns, as we're going to
introduce
a multi-column widget next.
Note that one definition of table_attach() now evaluates "top" twice.
Preventing that would be a mess: we could be tempted to introduce a do {
... } while (0) block with a block-scope auto-storage variable. However,
it could trigger gcc warnings (about shadowing other variables), in which
case we'd end up copying NBDKIT_UNIQUE_NAME() from nbdkit, which is
totally overkill. So just evaluate "top" twice; it's not a complex
or very
widely used macro. (BTW, the previous replacement text of the *other*
definition of table_attach() used to evaluate "top" twice as well.)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---
Notes:
v2:
- s/colums/columns/ in the commit message [Rich]
- pick up Rich's R-b
gui-gtk3-compat.h | 8 +--
gui.c | 69 ++++++++++++--------
2 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/gui-gtk3-compat.h b/gui-gtk3-compat.h
index 5388c3a3530c..e26506469eea 100644
--- a/gui-gtk3-compat.h
+++ b/gui-gtk3-compat.h
@@ -63,7 +63,7 @@ typedef enum
*/
#define table_new(grid, rows, columns) \
(grid) = gtk_grid_new ()
-#define table_attach(grid, child, left, right, top, bottom, xoptions, yoptions,
xpadding, ypadding) \
+#define table_attach(grid, child, left, right, top, xoptions, yoptions,
xpadding, ypadding) \
do { \
if (((xoptions) & GTK_EXPAND) != 0) \
gtk_widget_set_hexpand ((child), TRUE); \
@@ -75,14 +75,14 @@ typedef enum
gtk_widget_set_valign ((child), GTK_ALIGN_FILL); \
set_padding ((child), (xpadding), (ypadding)); \
gtk_grid_attach (GTK_GRID (grid), (child), \
- (left), (top), (right)-(left), (bottom)-(top)); \
+ (left), (top), (right)-(left), 1); \
} while (0)
#else
#define table_new(table, rows, columns) \
(table) = gtk_table_new ((rows), (columns), FALSE)
-#define table_attach(table, child, left, right,top, bottom, xoptions, yoptions,
xpadding, ypadding) \
+#define table_attach(table, child, left, right,top, xoptions, yoptions,
xpadding, ypadding) \
gtk_table_attach (GTK_TABLE (table), (child), \
- (left), (right), (top), (bottom), \
+ (left), (right), (top), (top + 1), \
(xoptions), (yoptions), (xpadding), (ypadding))
#endif
diff --git a/gui.c b/gui.c
index b4a9fed18410..eeb15a096351 100644
--- a/gui.c
+++ b/gui.c
@@ -210,6 +210,7 @@ create_connection_dialog (struct config *config)
GtkWidget *configure_network;
GtkWidget *xterm;
char port_str[64];
+ int row;
conn_dlg = gtk_dialog_new ();
gtk_window_set_title (GTK_WINDOW (conn_dlg), g_get_prgname ());
@@ -221,9 +222,10 @@ create_connection_dialog (struct config *config)
set_padding (intro, 10, 10);
table_new (table, 5, 2);
+ row = 0;
server_label = gtk_label_new_with_mnemonic (_("Conversion
_server:"));
table_attach (table, server_label,
- 0, 1, 0, 1, GTK_FILL, GTK_FILL, 4, 4);
+ 0, 1, row, GTK_FILL, GTK_FILL, 4, 4);
set_alignment (server_label, 1., 0.5);
hbox_new (server_hbox, FALSE, 4);
@@ -240,11 +242,12 @@ create_connection_dialog (struct config *config)
gtk_box_pack_start (GTK_BOX (server_hbox), port_colon_label, FALSE, FALSE,
0);
gtk_box_pack_start (GTK_BOX (server_hbox), port_entry, FALSE, FALSE, 0);
table_attach (table, server_hbox,
- 1, 2, 0, 1, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ 1, 2, row, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ row++;
username_label = gtk_label_new_with_mnemonic (_("_User name:"));
table_attach (table, username_label,
- 0, 1, 1, 2, GTK_FILL, GTK_FILL, 4, 4);
+ 0, 1, row, GTK_FILL, GTK_FILL, 4, 4);
set_alignment (username_label, 1., 0.5);
username_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (username_label), username_entry);
@@ -253,11 +256,12 @@ create_connection_dialog (struct config *config)
else
gtk_entry_set_text (GTK_ENTRY (username_entry), "root");
table_attach (table, username_entry,
- 1, 2, 1, 2, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ 1, 2, row, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ row++;
password_label = gtk_label_new_with_mnemonic (_("_Password:"));
table_attach (table, password_label,
- 0, 1, 2, 3, GTK_FILL, GTK_FILL, 4, 4);
+ 0, 1, row, GTK_FILL, GTK_FILL, 4, 4);
set_alignment (password_label, 1., 0.5);
password_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (password_label), password_entry);
@@ -269,25 +273,27 @@ create_connection_dialog (struct config *config)
if (config->auth.password != NULL)
gtk_entry_set_text (GTK_ENTRY (password_entry), config->auth.password);
table_attach (table, password_entry,
- 1, 2, 2, 3, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ 1, 2, row, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ row++;
identity_label = gtk_label_new_with_mnemonic (_("SSH _Identity
URL:"));
table_attach (table, identity_label,
- 0, 1, 3, 4, GTK_FILL, GTK_FILL, 4, 4);
+ 0, 1, row, GTK_FILL, GTK_FILL, 4, 4);
set_alignment (identity_label, 1., 0.5);
identity_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (identity_label), identity_entry);
if (config->auth.identity.url != NULL)
gtk_entry_set_text (GTK_ENTRY (identity_entry),
config->auth.identity.url);
table_attach (table, identity_entry,
- 1, 2, 3, 4, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ 1, 2, row, GTK_EXPAND|GTK_FILL, GTK_FILL, 4, 4);
+ row++;
sudo_button gtk_check_button_new_with_mnemonic (_("Use su_do when
running virt-v2v"));
gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (sudo_button),
config->auth.sudo);
table_attach (table, sudo_button,
- 1, 2, 4, 5, GTK_FILL, GTK_FILL, 4, 4);
+ 1, 2, row, GTK_FILL, GTK_FILL, 4, 4);
hbox_new (test_hbox, FALSE, 0);
test = gtk_button_new_with_mnemonic (_("_Test connection"));
@@ -729,6 +735,7 @@ create_conversion_dialog (struct config *config)
GtkWidget *interfaces_frame, *interfaces_sw;
char vcpus_str[64];
char memory_str[64];
+ int row;
conv_dlg = gtk_dialog_new ();
gtk_window_set_title (GTK_WINDOW (conv_dlg), g_get_prgname ());
@@ -750,35 +757,38 @@ create_conversion_dialog (struct config *config)
vbox_new (target_vbox, FALSE, 1);
table_new (target_tbl, 3, 3);
+ row = 0;
guestname_label = gtk_label_new_with_mnemonic (_("_Name:"));
table_attach (target_tbl, guestname_label,
- 0, 1, 0, 1, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (guestname_label, 1., 0.5);
guestname_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (guestname_label), guestname_entry);
if (config->guestname != NULL)
gtk_entry_set_text (GTK_ENTRY (guestname_entry), config->guestname);
table_attach (target_tbl, guestname_entry,
- 1, 2, 0, 1, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+ row++;
vcpus_label = gtk_label_new_with_mnemonic (_("# _vCPUs:"));
table_attach (target_tbl, vcpus_label,
- 0, 1, 1, 2, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (vcpus_label, 1., 0.5);
vcpus_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (vcpus_label), vcpus_entry);
snprintf (vcpus_str, sizeof vcpus_str, "%d",
config->vcpu.cores);
gtk_entry_set_text (GTK_ENTRY (vcpus_entry), vcpus_str);
table_attach (target_tbl, vcpus_entry,
- 1, 2, 1, 2, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
vcpus_warning = gtk_image_new_from_stock (GTK_STOCK_DIALOG_WARNING,
GTK_ICON_SIZE_BUTTON);
table_attach (target_tbl, vcpus_warning,
- 2, 3, 1, 2, 0, 0, 1, 1);
+ 2, 3, row, 0, 0, 1, 1);
+ row++;
memory_label = gtk_label_new_with_mnemonic (_("_Memory (MB):"));
table_attach (target_tbl, memory_label,
- 0, 1, 2, 3, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (memory_label, 1., 0.5);
memory_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (memory_label), memory_entry);
@@ -786,11 +796,11 @@ create_conversion_dialog (struct config *config)
config->memory / 1024 / 1024);
gtk_entry_set_text (GTK_ENTRY (memory_entry), memory_str);
table_attach (target_tbl, memory_entry,
- 1, 2, 2, 3, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
memory_warning = gtk_image_new_from_stock (GTK_STOCK_DIALOG_WARNING,
GTK_ICON_SIZE_BUTTON);
table_attach (target_tbl, memory_warning,
- 2, 3, 2, 3, 0, 0, 1, 1);
+ 2, 3, row, 0, 0, 1, 1);
gtk_box_pack_start (GTK_BOX (target_vbox), target_tbl, TRUE, TRUE, 0);
@@ -809,20 +819,22 @@ create_conversion_dialog (struct config *config)
vbox_new (output_vbox, FALSE, 1);
table_new (output_tbl, 5, 2);
+ row = 0;
o_label = gtk_label_new_with_mnemonic (_("Output _to (-o):"));
table_attach (output_tbl, o_label,
- 0, 1, 0, 1, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (o_label, 1., 0.5);
o_combo = gtk_combo_box_text_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (o_label), o_combo);
gtk_widget_set_tooltip_markup (o_combo, _("<b>libvirt</b>
means send the converted guest to libvirt-managed KVM on the conversion server.
<b>local</b> means put it in a directory on the conversion server.
<b>rhv</b> means write it to RHV-M/oVirt. <b>glance</b>
means write it to OpenStack Glance. See the virt-v2v(1) manual page for more
information about output options."));
repopulate_output_combo (config);
table_attach (output_tbl, o_combo,
- 1, 2, 0, 1, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+ row++;
oc_label = gtk_label_new_with_mnemonic (_("_Output conn. (-oc):"));
table_attach (output_tbl, oc_label,
- 0, 1, 1, 2, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (oc_label, 1., 0.5);
oc_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (oc_label), oc_entry);
@@ -830,11 +842,12 @@ create_conversion_dialog (struct config *config)
if (config->output.connection != NULL)
gtk_entry_set_text (GTK_ENTRY (oc_entry), config->output.connection);
table_attach (output_tbl, oc_entry,
- 1, 2, 1, 2, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+ row++;
os_label = gtk_label_new_with_mnemonic (_("Output _storage
(-os):"));
table_attach (output_tbl, os_label,
- 0, 1, 2, 3, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (os_label, 1., 0.5);
os_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (os_label), os_entry);
@@ -842,11 +855,12 @@ create_conversion_dialog (struct config *config)
if (config->output.storage != NULL)
gtk_entry_set_text (GTK_ENTRY (os_entry), config->output.storage);
table_attach (output_tbl, os_entry,
- 1, 2, 2, 3, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+ row++;
of_label = gtk_label_new_with_mnemonic (_("Output _format
(-of):"));
table_attach (output_tbl, of_label,
- 0, 1, 3, 4, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (of_label, 1., 0.5);
of_entry = gtk_entry_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (of_label), of_entry);
@@ -854,11 +868,12 @@ create_conversion_dialog (struct config *config)
if (config->output.format != NULL)
gtk_entry_set_text (GTK_ENTRY (of_entry), config->output.format);
table_attach (output_tbl, of_entry,
- 1, 2, 3, 4, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+ row++;
oa_label = gtk_label_new_with_mnemonic (_("Output _allocation
(-oa):"));
table_attach (output_tbl, oa_label,
- 0, 1, 4, 5, GTK_FILL, GTK_FILL, 1, 1);
+ 0, 1, row, GTK_FILL, GTK_FILL, 1, 1);
set_alignment (oa_label, 1., 0.5);
oa_combo = gtk_combo_box_text_new ();
gtk_label_set_mnemonic_widget (GTK_LABEL (oa_label), oa_combo);
@@ -875,7 +890,7 @@ create_conversion_dialog (struct config *config)
break;
}
table_attach (output_tbl, oa_combo,
- 1, 2, 4, 5, GTK_FILL, GTK_FILL, 1, 1);
+ 1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
gtk_box_pack_start (GTK_BOX (output_vbox), output_tbl, TRUE, TRUE, 0);
gtk_container_add (GTK_CONTAINER (output_frame), output_vbox);
Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 4/6] gui: offer copying the vCPU topology from the fully populated physical one
Introduce a checkbox that lets the user choose between options (a) and
(b), as outlined in one of the previous patches in this series. When the
checkbox is ticked -- that is, the fully populated physical topology is
copied --, make the VCPU count text entry insensitive, and overwrite its
contents with the value calculated from the physical topology. Upon
unticking the checkbox, restore the last manual entry.
The VCPU limit warning applies automatically when the checkbox is toggled,
because when the VCPU count text entry is programmatically modified, the
entry emits the "changed" signal, and that chains to
vcpus_or_memory_check_callback().
However, in show_conversion_dialog(), we need to handle the case of option
(a) potentially becoming the default. In that case we have to render the
VCPU count text entry insensitive immediately, and overwrite its contents
(set originally from the *online* logical processor count, or on the
kernel command line) with the full population count derived from the
topology. For this, call vcpu_topo_toggled() manually. This in turn may
(as it should) trigger the VCPU limit warning at once, handled by the
vcpus_or_memory_check_callback() call right after.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---
Notes:
v2:
- pick up Rich's R-b
- s/dense_topo/phys_topo/ everywhere [Rich]
gui.c | 47 +++++++++++++++++++-
1 file changed, 45 insertions(+), 2 deletions(-)
diff --git a/gui.c b/gui.c
index eeb15a096351..0e90235f7dfe 100644
--- a/gui.c
+++ b/gui.c
@@ -121,12 +121,13 @@ static GtkWidget *conn_dlg,
/* The conversion dialog. */
static GtkWidget *conv_dlg,
- *guestname_entry, *vcpus_entry, *memory_entry,
+ *guestname_entry, *vcpu_topo, *vcpus_entry, *memory_entry,
*vcpus_warning, *memory_warning, *target_warning_label,
*o_combo, *oc_entry, *os_entry, *of_entry, *oa_combo,
*info_label,
*disks_list, *removable_list, *interfaces_list,
*start_button;
+static int vcpus_entry_when_last_sensitive;
/* The running dialog which is displayed when virt-v2v is running. */
static GtkWidget *run_dlg,
@@ -690,8 +691,10 @@ static void set_removable_from_ui (struct config *);
static void set_interfaces_from_ui (struct config *);
static void conversion_back_clicked (GtkWidget *w, gpointer data);
static void start_conversion_clicked (GtkWidget *w, gpointer data);
+static void vcpu_topo_toggled (GtkWidget *w, gpointer data);
static void vcpus_or_memory_check_callback (GtkWidget *w, gpointer data);
static void notify_ui_callback (int type, const char *data);
+static bool get_phys_topo_from_conv_dlg (void);
static int get_vcpus_from_conv_dlg (void);
static uint64_t get_memory_from_conv_dlg (void);
@@ -756,7 +759,7 @@ create_conversion_dialog (struct config *config)
vbox_new (target_vbox, FALSE, 1);
- table_new (target_tbl, 3, 3);
+ table_new (target_tbl, 4, 3);
row = 0;
guestname_label = gtk_label_new_with_mnemonic (_("_Name:"));
table_attach (target_tbl, guestname_label,
@@ -769,6 +772,13 @@ create_conversion_dialog (struct config *config)
table_attach (target_tbl, guestname_entry,
1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+ row++;
+ vcpu_topo = gtk_check_button_new_with_mnemonic (
+ _("Copy fully populated _pCPU topology"));
+ gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (vcpu_topo),
+ config->vcpu.phys_topo);
+ table_attach (target_tbl, vcpu_topo, 0, 2, row, GTK_FILL, GTK_FILL, 1, 1);
+
row++;
vcpus_label = gtk_label_new_with_mnemonic (_("# _vCPUs:"));
table_attach (target_tbl, vcpus_label,
@@ -778,6 +788,7 @@ create_conversion_dialog (struct config *config)
gtk_label_set_mnemonic_widget (GTK_LABEL (vcpus_label), vcpus_entry);
snprintf (vcpus_str, sizeof vcpus_str, "%d",
config->vcpu.cores);
gtk_entry_set_text (GTK_ENTRY (vcpus_entry), vcpus_str);
+ vcpus_entry_when_last_sensitive = config->vcpu.cores;
table_attach (target_tbl, vcpus_entry,
1, 2, row, GTK_FILL, GTK_FILL, 1, 1);
vcpus_warning = gtk_image_new_from_stock (GTK_STOCK_DIALOG_WARNING,
@@ -970,6 +981,8 @@ create_conversion_dialog (struct config *config)
G_CALLBACK (conversion_back_clicked), NULL);
g_signal_connect (G_OBJECT (start_button), "clicked",
G_CALLBACK (start_conversion_clicked), config);
+ g_signal_connect (G_OBJECT (vcpu_topo), "toggled",
+ G_CALLBACK (vcpu_topo_toggled), NULL);
g_signal_connect (G_OBJECT (vcpus_entry), "changed",
G_CALLBACK (vcpus_or_memory_check_callback), NULL);
g_signal_connect (G_OBJECT (memory_entry), "changed",
@@ -988,6 +1001,7 @@ show_conversion_dialog (void)
/* Show the conversion dialog. */
gtk_widget_show_all (conv_dlg);
+ vcpu_topo_toggled (NULL, NULL);
vcpus_or_memory_check_callback (NULL, NULL);
/* output_drivers may have been updated, so repopulate o_combo. */
@@ -1534,6 +1548,28 @@ concat_warning (char *warning, const char *fs, ...)
return warning;
}
+static void
+vcpu_topo_toggled (GtkWidget *w, gpointer data)
+{
+ bool phys_topo;
+ unsigned vcpus;
+ char vcpus_str[64];
+
+ phys_topo = get_phys_topo_from_conv_dlg ();
+ if (phys_topo) {
+ struct cpu_topo topo;
+
+ get_cpu_topology (&topo);
+ vcpus = topo.sockets * topo.cores * topo.threads;
+ vcpus_entry_when_last_sensitive = get_vcpus_from_conv_dlg ();
+ } else
+ vcpus = vcpus_entry_when_last_sensitive;
+
+ snprintf (vcpus_str, sizeof vcpus_str, "%u", vcpus);
+ gtk_entry_set_text (GTK_ENTRY (vcpus_entry), vcpus_str);
+ gtk_widget_set_sensitive (vcpus_entry, !phys_topo);
+}
+
/**
* Display a warning if the vCPUs or memory is outside the supported
* range (L<https://bugzilla.redhat.com/823758>).
@@ -1577,6 +1613,12 @@ vcpus_or_memory_check_callback (GtkWidget *w, gpointer
data)
gtk_label_set_text (GTK_LABEL (target_warning_label), "");
}
+static bool
+get_phys_topo_from_conv_dlg (void)
+{
+ return gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (vcpu_topo));
+}
+
static int
get_vcpus_from_conv_dlg (void)
{
@@ -2025,6 +2067,7 @@ start_conversion_clicked (GtkWidget *w, gpointer data)
return;
}
+ config->vcpu.phys_topo = get_phys_topo_from_conv_dlg ();
config->vcpu.cores = get_vcpus_from_conv_dlg ();
config->memory = get_memory_from_conv_dlg ();
Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 5/6] copy fully populated vCPU topology by default
Such a default seems reasonable for most phys-to-virt conversions.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Acked-by: Richard W.M. Jones <rjones at redhat.com>
---
Notes:
v2:
- pick up Rich's A-b
- resolve rebase conflicts due to dense_topo -> phys_topo renaming
- we can drop this patch, per Daniel's comment
<https://listman.redhat.com/archives/libguestfs/2022-September/029841.html>
generate-p2v-config.pl | 2 +-
main.c | 2 +-
test-virt-p2v-cmdline.sh | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl
index 47487f7b0f35..625f62ad399e 100755
--- a/generate-p2v-config.pl
+++ b/generate-p2v-config.pl
@@ -238,7 +238,7 @@ use a randomly generated name.",
shortopt => "", # ignored for booleans
description => "
Copy the physical machine's complete CPU topology (sockets, cores and
-threads) to the guest. Disabled by default. If disabled, the
+threads) to the guest. This is the default. If disabled, the
C<p2v.vcpu.cores> setting takes effect.",
),
"p2v.vcpu.cores" => manual_entry->new(
diff --git a/main.c b/main.c
index 6c44183b65b3..0fc837b4e0aa 100644
--- a/main.c
+++ b/main.c
@@ -291,7 +291,7 @@ set_config_defaults (struct config *config)
}
config->guestname = strdup (hostname);
- config->vcpu.phys_topo = false;
+ config->vcpu.phys_topo = true;
/* Defaults for #vcpus and memory are taken from the physical machine. */
i = sysconf (_SC_NPROCESSORS_ONLN);
diff --git a/test-virt-p2v-cmdline.sh b/test-virt-p2v-cmdline.sh
index 79c3061b1e7c..d14302e65b4f 100755
--- a/test-virt-p2v-cmdline.sh
+++ b/test-virt-p2v-cmdline.sh
@@ -26,7 +26,7 @@ out=test-virt-p2v-cmdline.out
rm -f $out
# The Linux kernel command line.
-$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user
p2v.password=secret p2v.skip_test_connection p2v.name=test p2v.vcpu.cores=4
p2v.memory=1G p2v.disks=sda,sdb,sdc p2v.removable=sdd p2v.interfaces=eth0,eth1
p2v.o=local p2v.oa=sparse p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp
p2v.network=em1:wired,other p2v.dump_config_and_exit' > $out
+$VG virt-p2v --cmdline='p2v.server=localhost p2v.port=123 p2v.username=user
p2v.password=secret p2v.skip_test_connection p2v.name=test
p2v.vcpu.phys_topo=false p2v.vcpu.cores=4 p2v.memory=1G p2v.disks=sda,sdb,sdc
p2v.removable=sdd p2v.interfaces=eth0,eth1 p2v.o=local p2v.oa=sparse
p2v.oc=qemu:///session p2v.of=raw p2v.os=/var/tmp p2v.network=em1:wired,other
p2v.dump_config_and_exit' > $out
# For debugging purposes.
cat $out
Laszlo Ersek
2022-Sep-08 16:27 UTC
[Libguestfs] [p2v PATCH v2 6/6] Makefile.am: set vCPU topology to 1*2*2 in the "p2v in a VM" tests
This lets us exercise both states of the "p2v.vcpu.phys_topo" switch
sensibly via the in-VM GUI.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Acked-by: Richard W.M. Jones <rjones at redhat.com>
---
Notes:
v2:
- pick up Rich's A-b
- s/dense_topo/phys_topo/ in the commit message [Rich]
Makefile.am | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Makefile.am b/Makefile.am
index f2fe0e3efc99..19c5f04c2fab 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -268,6 +268,7 @@ run-virt-p2v-in-a-vm: virt-p2v.img $(PHYSICAL_MACHINE)
$(shell guestfish get-hv) \
-M pc,accel=kvm:tcg \
-cpu host \
+ -smp sockets=1,cores=2,threads=2 \
-m 2048 \
-drive id=hd0,file=$(PHYSICAL_MACHINE),format=raw,if=ide \
-device qemu-xhci \
@@ -287,6 +288,7 @@ run-virt-p2v-in-an-nvme-vm: virt-p2v.img $(PHYSICAL_MACHINE)
$(BLANK_DISK)
$(shell guestfish get-hv) \
-M pc,accel=kvm:tcg \
-cpu host \
+ -smp sockets=1,cores=2,threads=2 \
-m 2048 \
-boot menu=on \
\
Laszlo Ersek
2022-Sep-09 09:10 UTC
[Libguestfs] [p2v PATCH v2 0/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1
On 09/08/22 18:27, Laszlo Ersek wrote:> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1590721 > v1: https://listman.redhat.com/archives/libguestfs/2022-September/029806.html > > Please see the Notes section on each patch for the updates in this > version (addressing v1 feedback). Patch #5 is a candidate for dropping > in particular. > > I'm also including a range-diff between v1 and v2, below. > > [...]I meant to append: v2 builds fine at every stage again, and I retested full conversions too, with both states of the new boolean knob. Laszlo