Re: [PATCH] virnetdevbandwidth.c: Put a limit to "quantum"

2024-04-25 Thread Peter Krempa
On Thu, Apr 25, 2024 at 08:34:57 +0200, Michal Privoznik wrote:
> The "quantum" attribute of HTB is documented as:
> 
>   Number of bytes to serve from this class before the scheduler
>   moves to the next class.
> 
> Since v1.3.2-rc1~225 we compute what we think is the appropriate
> value and pass it on the TC command line. But kernel and
> subsequently TC use uint32_t to store this value. If we compute
> value outside of this type then TC fails and prints usage which
> we then interpret as an error message. Needlessly long error
> message. While there's not much we can do about the latter, we
> can put a cap on the value and stop tickling this behavior of TC.
> 
> Fixes: 065054daa71f645fc83aff0271f194d326208616
> Resolves: https://issues.redhat.com/browse/RHEL-34112
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virnetdevbandwidth.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
> index ec41666f67..7f5714a33f 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c
> @@ -46,6 +46,7 @@ virNetDevBandwidthCmdAddOptimalQuantum(virCommand *cmd,
> const virNetDevBandwidthRate *rate)
>  {
>  const unsigned long long mtu = 1500;
> +const unsigned long long r2q_limit = (1ULL << 32) -1;

This quacks like  'UINT32_MAX'

>  unsigned long long r2q;
>  
>  /* When two or more classes compete for unused bandwidth they are each
> @@ -60,6 +61,11 @@ virNetDevBandwidthCmdAddOptimalQuantum(virCommand *cmd,
>  if (!r2q)
>  r2q = 1;
>  
> +/* But there's an internal limit in TC (well, kernel's implementation of
> + * HTB) for quantum: it has to fit into u32. Put a cap there. */
> +if (r2q > r2q_limit)
> +r2q = r2q_limit;

And you could use the constant directly here, which would also address
the broken spacing around subtraction in the variable you've added.

Reviewed-by: Peter Krempa 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 3/4] qemu: validate machine virt ras feature

2024-04-25 Thread Michal Prívozník
On 4/24/24 16:59, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova 
> ---
>  src/qemu/qemu_validate.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index b33618b494..c8bee6f23d 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -69,6 +69,7 @@ qemuValidateDomainDefPSeriesFeature(const virDomainDef *def,
>  case VIR_DOMAIN_FEATURE_HTM:
>  case VIR_DOMAIN_FEATURE_NESTED_HV:
>  case VIR_DOMAIN_FEATURE_CCF_ASSIST:
> +case VIR_DOMAIN_FEATURE_RAS:
>  if (!virTristateSwitchTypeToString(def->features[feature])) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Invalid setting for pseries feature '%1$s'"),
> @@ -227,6 +228,20 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_RAS:
> +if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
> +!qemuDomainIsARMVirt(def)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("ras feature is only supported with ARM 
> Virt machines"));
> +return -1;
> +}
> +if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_RAS)) {

Just realized, this checks for the capability only when:

  

  

But the next patch adds 'ras=' onto cmd line even for the following case:

  

But unfortunately, I don't think we have a good way out. How I see our
options:

1) check for != VIR_TRISTATE_SWITCH_ABSENT here, just like we're doing
in the next patch. Problem is, when user specifies 
and we're talking to an older QEMU (say 4.2.0), which doesn't support
(toggling) the feature, well then users are would be unable to start
such guest. Even though the feature might already be off by default.

2) change check in the next patch to == VIR_TRISTATE_SWITCH_ON, just
like we're doing here. This means, we're effectively able to just format
ras=on onto the cmd line. Well, what if the default changes in the
future (say QEMU-10.0.0) and users want to turn it off?

3) do nothing and keep both patches as they are.

Looking around other features (VIR_DOMAIN_FEATURE_CCF_ASSIST,
VIR_DOMAIN_FEATURE_NESTED_HV, ...) - they suffer the same.

Let's stick with the option 3) then.

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 0/4] implement 'ras' feature support

2024-04-25 Thread Michal Prívozník
On 4/24/24 16:59, Kristina Hanicova wrote:
> *** BLURB HERE ***
> 
> Kristina Hanicova (4):
>   Introduce QEMU_CAPS_MACHINE_VIRT_RAS capability
>   conf: parse and format machine virt ras feature
>   qemu: validate machine virt ras feature
>   qemu: format machine virt ras feature and test it
> 
>  docs/formatdomain.rst |  5 +++
>  src/conf/domain_conf.c|  6 +++-
>  src/conf/domain_conf.h|  1 +
>  src/conf/schemas/domaincommon.rng |  5 +++
>  src/qemu/qemu_capabilities.c  |  2 ++
>  src/qemu/qemu_capabilities.h  |  1 +
>  src/qemu/qemu_command.c   |  5 +++
>  src/qemu/qemu_validate.c  | 16 ++
>  .../caps_5.2.0_aarch64.xml|  1 +
>  .../caps_6.0.0_aarch64.xml|  1 +
>  .../caps_6.2.0_aarch64.xml|  1 +
>  .../caps_7.0.0_aarch64+hvf.xml|  1 +
>  .../caps_7.0.0_aarch64.xml|  1 +
>  .../caps_8.2.0_aarch64.xml|  1 +
>  .../caps_8.2.0_armv7l.xml |  1 +
>  .../aarch64-features-ras.aarch64-latest.args  | 31 +++
>  .../aarch64-features-ras.aarch64-latest.xml   |  1 +
>  .../qemuxmlconfdata/aarch64-features-ras.xml  | 26 
>  tests/qemuxmlconftest.c   |  2 ++
>  19 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 
> tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.args
>  create mode 12 
> tests/qemuxmlconfdata/aarch64-features-ras.aarch64-latest.xml
>  create mode 100644 tests/qemuxmlconfdata/aarch64-features-ras.xml
> 

Reviewed-by: Michal Privoznik 

But I'll postpone pushing these for a bit to give others a chance to
express their position on the problem I'm mentioning in 3/4.

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v2 0/3] qemu: Add support for virtio sound model

2024-04-25 Thread Rayhan Faizel
Ping v2

On Thu, Apr 18, 2024 at 11:48 AM Rayhan Faizel  wrote:
>
> Hi,
>
> A week has passed, so I'm bumping this.



-- 
Rayhan Faizel
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 0/2] cmdComplete: Fix two memleaks

2024-04-25 Thread Peter Krempa
Putting my recently-reviewed series through CI revealed that there were
two pre-existing memleaks in the completion code caught by the test
additions.

Peter Krempa (2):
  vsh: cmdComplete: Don't leak buffer for completion
  vshReadlineInit: Initialize only once

 tools/vsh.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 1/2] vsh: cmdComplete: Don't leak buffer for completion

2024-04-25 Thread Peter Krempa
The buffer which we assign to the 'rl_line_buffer' variable of readline
would be overwritten and thus leaked on multiple invocations of
cmdComplete in one session.

Free/clear it after it's used.

Hitting this leak was until recenly possible only in non-interactive
batch mode and recently also in interactive mode as 'complete' can be
used multiple times now interactively.

Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
Signed-off-by: Peter Krempa 
---
 tools/vsh.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 2805574ec6..05de54b5b0 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3439,7 +3439,10 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
  * In our case it's at the end of the whole line. */
 rl_point = strlen(rl_line_buffer);

-if (!(matches = vshReadlineCompletion(arg, 0, 0)))
+matches = vshReadlineCompletion(arg, 0, 0);
+g_clear_pointer(&rl_line_buffer, g_free);
+
+if (!matches)
 return false;

 for (iter = matches; *iter; iter++) {
-- 
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 2/2] vshReadlineInit: Initialize only once

2024-04-25 Thread Peter Krempa
'vshReadlineInit' is called when interactive virsh is started but also
on each call to 'cmdComplete'. Calling it repeatedly (using the
'complete' command interactively, or multiple times in batch mode) leaks
the buffers for history file configuration.

Avoid multiple setups of this function by returning success in case the
history file config is already present.

Signed-off-by: Peter Krempa 
---
 tools/vsh.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/tools/vsh.c b/tools/vsh.c
index 05de54b5b0..f954f7af77 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2908,6 +2908,10 @@ vshReadlineInit(vshControl *ctl)
 const char *break_characters = " \t\n`@$><=;|&{(";
 const char *quote_characters = "\"'";

+/* initialize readline stuff only once */
+if (ctl->historydir)
+return 0;
+
 /* Opaque data for autocomplete callbacks. */
 autoCompleteOpaque = ctl;

-- 
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 05/13] virshtest: Add test cases for command completion helper

2024-04-25 Thread Peter Krempa
On Fri, Apr 19, 2024 at 15:05:23 +0200, Peter Krempa wrote:
> Add both single invocations as well as a script containing the same
> commands.
> 
> Signed-off-by: Peter Krempa 
> ---

[...]

> diff --git a/tests/virshtest.c b/tests/virshtest.c
> index 14a96f2d35..869ecbb358 100644
> --- a/tests/virshtest.c
> +++ b/tests/virshtest.c
> @@ -342,6 +342,43 @@ mymain(void)
>   "checkpoint-create test --redefine checkpoint-c2.xml ;"
>   "checkpoint-info test c2");
> 
> +DO_TEST_FULL("completion-command", NULL, VIRSH_CUSTOM,
> + "complete", "--", "ech");
> +DO_TEST_FULL("completion-command-complete", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo");
> +DO_TEST_FULL("completion-args", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo", "");
> +DO_TEST_FULL("completion-arg-partial", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo", "--xm", "--s");
> +DO_TEST_FULL("completion-arg-full-bool", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo", "--nonexistant-arg", "--xml");
> +DO_TEST_FULL("completion-arg-full-bool-next", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo", "--nonexistant-arg", "--xml", "");
> +DO_TEST_FULL("completion-arg-full-string", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo", "--nonexistant-arg", "--prefix");
> +DO_TEST_FULL("completion-arg-full-string-next", NULL, VIRSH_CUSTOM,
> + "complete", "--", "echo", "--nonexistant-arg", "--prefix", 
> "");
> +DO_TEST_FULL("completion-arg-full-argv", NULL, VIRSH_CUSTOM,
> + "complete", "--", "domstats", "--domain");
> +DO_TEST_FULL("completion-arg-full-argv-next", NULL, VIRSH_DEFAULT,
> + "complete", "--", "domstats", "--domain", "");
> +DO_TEST_FULL("completion-argv-multiple", NULL, VIRSH_CUSTOM,
> + "complete", "--", "domstats", "--domain", "fc", "--domain", 
> "fv");
> +DO_TEST_FULL("completion-argv-multiple-next", NULL, VIRSH_CUSTOM,
> + "complete", "--", "domstats", "--domain", "fv", "--domain", 
> "fc", "");
> +DO_TEST_FULL("completion-argv-multiple-positional", NULL, VIRSH_CUSTOM,
> + "complete", "--", "domstats", "--domain", "fc", "fv");
> +DO_TEST_FULL("completion-argv-multiple-positional-next", NULL, 
> VIRSH_CUSTOM,
> + "complete", "--", "domstats", "--domain", "fc", "fv", "");
> +DO_TEST_FULL("completion-arg-positional-empty", NULL, VIRSH_CUSTOM,
> + "complete", "--", "domstate", "");
> +DO_TEST_FULL("completion-arg-positional-partial", NULL, VIRSH_CUSTOM,
> + "complete", "--", "domstate", "fv");
> +DO_TEST_FULL("completion-arg-positional-partial-next", NULL, 
> VIRSH_CUSTOM,
> + "complete", "--", "domstate", "fv", "");
> +
> +DO_TEST_SCRIPT("completion", NULL, VIRSH_DEFAULT);

Running this trhough CI revealed that I forgot to skip these when
readline is not compiled in, which I'll squash into this patch.

Additionally two memleaks were found in the original code revealed by
this patch, which I've sent patches for.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH RFC v2 00/12] Support throttle block filters

2024-04-25 Thread Peter Krempa
On Mon, Apr 22, 2024 at 17:28:11 +0800, Chun Feng Wu wrote:
> Hi,
> 
> May I know if this new version code is under review?

I didn't yet get to reviewing this, but I didn't forget about it.

Note that it's a complex feature, adding lot of code and has the
possibility to break VMs, thus I need to find a sizable chunk of time to
dig into it.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 0/2] cmdComplete: Fix two memleaks

2024-04-25 Thread Ján Tomko

On a Thursday in 2024, Peter Krempa wrote:

Putting my recently-reviewed series through CI revealed that there were
two pre-existing memleaks in the completion code caught by the test
additions.

Peter Krempa (2):
 vsh: cmdComplete: Don't leak buffer for completion
 vshReadlineInit: Initialize only once

tools/vsh.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 3/4] qemu: validate machine virt ras feature

2024-04-25 Thread Ján Tomko

On a Thursday in 2024, Michal Prívozník wrote:

On 4/24/24 16:59, Kristina Hanicova wrote:

Signed-off-by: Kristina Hanicova 
---
 src/qemu/qemu_validate.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index b33618b494..c8bee6f23d 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -69,6 +69,7 @@ qemuValidateDomainDefPSeriesFeature(const virDomainDef *def,
 case VIR_DOMAIN_FEATURE_HTM:
 case VIR_DOMAIN_FEATURE_NESTED_HV:
 case VIR_DOMAIN_FEATURE_CCF_ASSIST:
+case VIR_DOMAIN_FEATURE_RAS:
 if (!virTristateSwitchTypeToString(def->features[feature])) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Invalid setting for pseries feature '%1$s'"),
@@ -227,6 +228,20 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
 break;

 case VIR_DOMAIN_FEATURE_RAS:
+if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+!qemuDomainIsARMVirt(def)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("ras feature is only supported with ARM Virt 
machines"));
+return -1;
+}
+if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_VIRT_RAS)) {


Just realized, this checks for the capability only when:

 
   
 

But the next patch adds 'ras=' onto cmd line even for the following case:

 

But unfortunately, I don't think we have a good way out. How I see our
options:

1) check for != VIR_TRISTATE_SWITCH_ABSENT here, just like we're doing
in the next patch. Problem is, when user specifies 
and we're talking to an older QEMU (say 4.2.0), which doesn't support
(toggling) the feature, well then users are would be unable to start
such guest. Even though the feature might already be off by default.



I prefer this one. Don't see any point in toggling a feature that:
1) was not even present in the QEMU they're using
2) is currently off by default and possibly will be for some time.


2) change check in the next patch to == VIR_TRISTATE_SWITCH_ON, just
like we're doing here. This means, we're effectively able to just format
ras=on onto the cmd line. Well, what if the default changes in the
future (say QEMU-10.0.0) and users want to turn it off?

3) do nothing and keep both patches as they are.



Worst of the three - if we're letting QEMU report the error, why bother
with the validation?

Jano


Looking around other features (VIR_DOMAIN_FEATURE_CCF_ASSIST,
VIR_DOMAIN_FEATURE_NESTED_HV, ...) - they suffer the same.

Let's stick with the option 3) then.

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH] vmx: Check serialX.vspc before serialX.fileName

2024-04-25 Thread Martin Kletzander
When using vSPC (Virtual Serial Port Concentrator) in vSphere the actual
address for it is saved in serialX.vspc in which case the
serialX.fileName is most probably something we can't get any useful
information from and we also fail during the parsing rendering any
dumpxml and similar tries unsuccessful.

Instead of parsing the vspc URL with something along the lines of
`virURIParse(vspc ? vspc : fileName)`, which could lead to us reporting
information that is very prune to misuse (the vSPC seemingly has a
protocol on top of the telnet connection; redefining the domain would
change the behaviour; the URL might have a fragment we are not saving;
etc.) or adding more XML knobs to indicate vSPC usage (which we would
not be able to configure; we'd have to properly error out everywhere;
etc.) let's just report dummy serial port that leads to nowhere.

Resolves: https://issues.redhat.com/browse/RHEL-32182
Signed-off-by: Martin Kletzander 
---
 src/vmx/vmx.c| 12 +++
 tests/vmx2xmldata/esx-in-the-wild-13.vmx | 97 
 tests/vmx2xmldata/esx-in-the-wild-13.xml | 55 ++
 tests/vmx2xmltest.c  |  1 +
 4 files changed, 165 insertions(+)
 create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.vmx
 create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.xml

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 5da67aae60d9..32074f62e14c 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2975,6 +2975,9 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int 
port,
 char fileName_name[48] = "";
 g_autofree char *fileName = NULL;
 
+char vspc_name[48] = "";
+g_autofree char *vspc = NULL;
+
 char network_endPoint_name[48] = "";
 g_autofree char *network_endPoint = NULL;
 
@@ -2997,6 +3000,7 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int 
port,
 VMX_BUILD_NAME(startConnected);
 VMX_BUILD_NAME(fileType);
 VMX_BUILD_NAME(fileName);
+VMX_BUILD_NAME(vspc);
 VMX_BUILD_NAME_EXTRA(network_endPoint, "network.endPoint");
 
 /* vmx:present */
@@ -3026,6 +3030,10 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int 
port,
 if (virVMXGetConfigString(conf, fileName_name, &fileName, true) < 0)
 goto cleanup;
 
+/* vmx:fileName -> def:data.file.path */
+if (virVMXGetConfigString(conf, vspc_name, &vspc, true) < 0)
+goto cleanup;
+
 /* vmx:network.endPoint -> def:data.tcp.listen */
 if (virVMXGetConfigString(conf, network_endPoint_name, &network_endPoint,
   true) < 0) {
@@ -3057,6 +3065,10 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, int 
port,
 (*def)->target.port = port;
 (*def)->source->type = VIR_DOMAIN_CHR_TYPE_PIPE;
 (*def)->source->data.file.path = g_steal_pointer(&fileName);
+} else if (STRCASEEQ(fileType, "network") && vspc) {
+(*def)->target.port = port;
+(*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE;
+(*def)->source->data.file.path = g_strdup("/dev/null");
 } else if (STRCASEEQ(fileType, "network")) {
 (*def)->target.port = port;
 (*def)->source->type = VIR_DOMAIN_CHR_TYPE_TCP;
diff --git a/tests/vmx2xmldata/esx-in-the-wild-13.vmx 
b/tests/vmx2xmldata/esx-in-the-wild-13.vmx
new file mode 100644
index ..1016acab28d8
--- /dev/null
+++ b/tests/vmx2xmldata/esx-in-the-wild-13.vmx
@@ -0,0 +1,97 @@
+.encoding = "UTF-8"
+config.version = "8"
+virtualHW.version = "19"
+vmci0.present = "TRUE"
+floppy0.present = "FALSE"
+memSize = "1024"
+tools.upgrade.policy = "manual"
+sched.cpu.units = "mhz"
+vm.createDate = "1704946351823519"
+scsi0.virtualDev = "lsilogic"
+scsi0.present = "TRUE"
+scsi0:0.deviceType = "scsi-hardDisk"
+scsi0:0.fileName = "Test-Mig-VM-1 
(01ce57d0-4e20-41a5-8b6c-bcbf49a032ec)_2.vmdk"
+sched.scsi0:0.shares = "normal"
+sched.scsi0:0.throughputCap = "off"
+scsi0:0.present = "TRUE"
+ethernet0.virtualDev = "vmxnet3"
+ethernet0.opaqueNetwork.id = "25c9a00e-dc60-4918-89b7-41c951988366"
+ethernet0.opaqueNetwork.type = "nsx.LogicalSwitch"
+ethernet0.shares = "normal"
+ethernet0.addressType = "static"
+ethernet0.address = "fa:16:3e:bb:2c:4a"
+ethernet0.externalId = "4b57523e-35af-4f8d-b050-1a7410e1a307"
+ethernet0.uptCompatibility = "TRUE"
+ethernet0.present = "TRUE"
+ethernet0.networkName = "Test"
+serial0.fileType = "network"
+serial0.fileName = 
"ZmVybmV0IGdBQUFBQUJrdFotaW8yclpkRXR6N3dBcDdyYkFMaWFUMVd4RENJSHgtUXpkTlMyTzRRejI3V192QVlOVUY3ZU1SOTNHZXJrN1dGb2stS0EybmpwWFQ4NjJNNlgwc2ZDdmNlOE50eFNhcU1XdlNBTmdhazQ1T1J3LUI5OEZsSDdwMDBZa2R6bWt4Y1Ax"
+serial0.vspc = 
"telnets://10.28.100.26:18979#thumbprint=18:F5:79:E5:73:A5:22:83:C0:57:B9:B4:FA:CE:60:19:F1:12:F5:7B"
+serial0.yieldOnMsrRead = "TRUE"
+serial0.present = "TRUE"
+displayName = "Test-Mig-VM-1 (01ce57d0-4e20-41a5-8b6c-bcbf49a032ec)"
+annotation = 
"name:Test-Mig-VM-1|0Auserid:962314ba515c48388a0e95c0961709ff|0Ausername:admin|0Aprojectid:b06b5f77b6bb442f85b1c67cff980ef9|0Aproj

[PATCH 0/2] ci : drop CentOS 8 Stream

2024-04-25 Thread Daniel P . Berrangé
CentOS 8 Stream goes EOL at the end of May, and unlike most other
distros, it will actively break the ability to install and update
it, as package repos are all archived. We thus have to stop using
it in CI, but fortunately Alma Linux 8 continues to give us enough
ongoing coverage.

The exception is the integration tests. To keep those working would
require the custom runner to be reinstalled with Alma Linux, which
I don't have access to do.

Daniel P. Berrangé (2):
  ci: refresh with latest lcitool manifest
  ci: drop CentOS 8 Stream and refresh

 ci/buildenv/alpine-319.sh|   2 +-
 ci/buildenv/alpine-edge.sh   |   2 +-
 ci/buildenv/centos-stream-8.sh   | 104 -
 ci/buildenv/centos-stream-9.sh   |   1 +
 ci/cirrus/build.yml  |   2 +-
 ci/containers/alpine-319.Dockerfile  |   2 +-
 ci/containers/alpine-edge.Dockerfile |   2 +-
 ci/containers/centos-stream-8.Dockerfile | 107 -
 ci/containers/centos-stream-9.Dockerfile |   1 +
 ci/gitlab/build-templates.yml| 184 ++-
 ci/gitlab/builds.yml |  15 --
 ci/gitlab/containers.yml |   7 -
 ci/integration.yml   |  24 ---
 ci/manifest.yml  |   8 -
 14 files changed, 90 insertions(+), 371 deletions(-)
 delete mode 100644 ci/buildenv/centos-stream-8.sh
 delete mode 100644 ci/containers/centos-stream-8.Dockerfile

-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 1/2] ci: refresh with latest lcitool manifest

2024-04-25 Thread Daniel P . Berrangé
This brings in a fix to the job rules which solves a problem with
jobs getting skipped in merge requests in some scenarios. It also
changes the way Cirrus CI vars are set, which involves a weak to
the way $PATH is set in build.yml.

Signed-off-by: Daniel P. Berrangé 
---
 ci/buildenv/alpine-319.sh|   2 +-
 ci/buildenv/alpine-edge.sh   |   2 +-
 ci/buildenv/centos-stream-9.sh   |   1 +
 ci/cirrus/build.yml  |   2 +-
 ci/containers/alpine-319.Dockerfile  |   2 +-
 ci/containers/alpine-edge.Dockerfile |   2 +-
 ci/containers/centos-stream-9.Dockerfile |   1 +
 ci/gitlab/build-templates.yml| 184 ++-
 8 files changed, 90 insertions(+), 106 deletions(-)

diff --git a/ci/buildenv/alpine-319.sh b/ci/buildenv/alpine-319.sh
index c1aedf79b8..43fcb38a46 100644
--- a/ci/buildenv/alpine-319.sh
+++ b/ci/buildenv/alpine-319.sh
@@ -69,7 +69,7 @@ function install_buildenv() {
 xen-dev \
 yajl-dev
 rm -f /usr/lib*/python3*/EXTERNALLY-MANAGED
-apk list | sort > /packages.txt
+apk list --installed | sort > /packages.txt
 mkdir -p /usr/libexec/ccache-wrappers
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang
diff --git a/ci/buildenv/alpine-edge.sh b/ci/buildenv/alpine-edge.sh
index c1aedf79b8..43fcb38a46 100644
--- a/ci/buildenv/alpine-edge.sh
+++ b/ci/buildenv/alpine-edge.sh
@@ -69,7 +69,7 @@ function install_buildenv() {
 xen-dev \
 yajl-dev
 rm -f /usr/lib*/python3*/EXTERNALLY-MANAGED
-apk list | sort > /packages.txt
+apk list --installed | sort > /packages.txt
 mkdir -p /usr/libexec/ccache-wrappers
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang
diff --git a/ci/buildenv/centos-stream-9.sh b/ci/buildenv/centos-stream-9.sh
index d5cfde517f..8dabda22b3 100644
--- a/ci/buildenv/centos-stream-9.sh
+++ b/ci/buildenv/centos-stream-9.sh
@@ -43,6 +43,7 @@ function install_buildenv() {
 libblkid-devel \
 libcap-ng-devel \
 libcurl-devel \
+libiscsi-devel \
 libnbd-devel \
 libnl3-devel \
 libpcap-devel \
diff --git a/ci/cirrus/build.yml b/ci/cirrus/build.yml
index 0ae5c2ce64..c0ac05f4d9 100644
--- a/ci/cirrus/build.yml
+++ b/ci/cirrus/build.yml
@@ -6,7 +6,7 @@ env:
   CI_COMMIT_REF_NAME: "@CI_COMMIT_REF_NAME@"
   CI_MERGE_REQUEST_REF_PATH: "@CI_MERGE_REQUEST_REF_PATH@"
   CI_COMMIT_SHA: "@CI_COMMIT_SHA@"
-  PATH: "@PATH@"
+  PATH: "@PATH_EXTRA@:$PATH"
   PKG_CONFIG_PATH: "@PKG_CONFIG_PATH@"
   PYTHON: "@PYTHON@"
   MAKE: "@MAKE@"
diff --git a/ci/containers/alpine-319.Dockerfile 
b/ci/containers/alpine-319.Dockerfile
index 1a7a5c4d69..2455184a87 100644
--- a/ci/containers/alpine-319.Dockerfile
+++ b/ci/containers/alpine-319.Dockerfile
@@ -70,7 +70,7 @@ RUN apk update && \
 xen-dev \
 yajl-dev && \
 rm -f /usr/lib*/python3*/EXTERNALLY-MANAGED && \
-apk list | sort > /packages.txt && \
+apk list --installed | sort > /packages.txt && \
 mkdir -p /usr/libexec/ccache-wrappers && \
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc && \
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang && \
diff --git a/ci/containers/alpine-edge.Dockerfile 
b/ci/containers/alpine-edge.Dockerfile
index dfe233a1d5..b28c96692c 100644
--- a/ci/containers/alpine-edge.Dockerfile
+++ b/ci/containers/alpine-edge.Dockerfile
@@ -70,7 +70,7 @@ RUN apk update && \
 xen-dev \
 yajl-dev && \
 rm -f /usr/lib*/python3*/EXTERNALLY-MANAGED && \
-apk list | sort > /packages.txt && \
+apk list --installed | sort > /packages.txt && \
 mkdir -p /usr/libexec/ccache-wrappers && \
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc && \
 ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang && \
diff --git a/ci/containers/centos-stream-9.Dockerfile 
b/ci/containers/centos-stream-9.Dockerfile
index 84465a8df9..082b18d06f 100644
--- a/ci/containers/centos-stream-9.Dockerfile
+++ b/ci/containers/centos-stream-9.Dockerfile
@@ -44,6 +44,7 @@ RUN dnf distro-sync -y && \
 libblkid-devel \
 libcap-ng-devel \
 libcurl-devel \
+libiscsi-devel \
 libnbd-devel \
 libnl3-devel \
 libpcap-devel \
diff --git a/ci/gitlab/build-templates.yml b/ci/gitlab/build-templates.yml
index 75d9a6f127..b1e41b0783 100644
--- a/ci/gitlab/build-templates.yml
+++ b/ci/gitlab/build-templates.yml
@@ -37,7 +37,7 @@
   variables:
 IMAGE: $CI_REGISTRY/$RUN_UPSTREAM_NAMESPACE/libvirt/ci-$NAME:latest
   rules:
-### Rules where we expect to use pre-built container images
+### PUSH events
 
 # upstream: pushes to the default branch
 - if: '$CI_PROJECT_NAMESPACE == $RUN_UPSTREAM_NAMESPACE && 
$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH && 
$JOB_OPTIONAL'
@@ -53,90 +53,89

[PATCH 2/2] ci: drop CentOS 8 Stream and refresh

2024-04-25 Thread Daniel P . Berrangé
This drops the CentOS 8 Stream distro target, since that is going EOL
at the end of May, at which point it will cease to be installable
due to package repos being archived.

Signed-off-by: Daniel P. Berrangé 
---
 ci/buildenv/centos-stream-8.sh   | 104 --
 ci/containers/centos-stream-8.Dockerfile | 107 ---
 ci/gitlab/builds.yml |  15 
 ci/gitlab/containers.yml |   7 --
 ci/integration.yml   |  24 -
 ci/manifest.yml  |   8 --
 6 files changed, 265 deletions(-)
 delete mode 100644 ci/buildenv/centos-stream-8.sh
 delete mode 100644 ci/containers/centos-stream-8.Dockerfile

diff --git a/ci/buildenv/centos-stream-8.sh b/ci/buildenv/centos-stream-8.sh
deleted file mode 100644
index 542b2a70ba..00
--- a/ci/buildenv/centos-stream-8.sh
+++ /dev/null
@@ -1,104 +0,0 @@
-# THIS FILE WAS AUTO-GENERATED
-#
-#  $ lcitool manifest ci/manifest.yml
-#
-# https://gitlab.com/libvirt/libvirt-ci
-
-function install_buildenv() {
-dnf distro-sync -y
-dnf install 'dnf-command(config-manager)' -y
-dnf config-manager --set-enabled -y powertools
-dnf install -y centos-release-advanced-virtualization
-dnf install -y epel-release
-dnf install -y epel-next-release
-dnf install -y \
-audit-libs-devel \
-augeas \
-bash-completion \
-ca-certificates \
-ccache \
-clang \
-cpp \
-cyrus-sasl-devel \
-device-mapper-devel \
-diffutils \
-dwarves \
-ebtables \
-firewalld-filesystem \
-fuse-devel \
-gcc \
-gettext \
-git \
-glib2-devel \
-glibc-devel \
-glibc-langpack-en \
-glusterfs-api-devel \
-gnutls-devel \
-grep \
-iproute \
-iproute-tc \
-iptables \
-iscsi-initiator-utils \
-kmod \
-libacl-devel \
-libattr-devel \
-libblkid-devel \
-libcap-ng-devel \
-libcurl-devel \
-libiscsi-devel \
-libnbd-devel \
-libnl3-devel \
-libpcap-devel \
-libpciaccess-devel \
-librbd-devel \
-libselinux-devel \
-libssh-devel \
-libssh2-devel \
-libtirpc-devel \
-libwsman-devel \
-libxml2 \
-libxml2-devel \
-libxslt \
-lvm2 \
-make \
-meson \
-netcf-devel \
-nfs-utils \
-ninja-build \
-numactl-devel \
-numad \
-parted-devel \
-perl \
-pkgconfig \
-polkit \
-python3 \
-python3-docutils \
-python3-flake8 \
-python3-pip \
-python3-pytest \
-python3-setuptools \
-python3-wheel \
-qemu-img \
-readline-devel \
-rpm-build \
-sanlock-devel \
-sed \
-systemd-devel \
-systemd-rpm-macros \
-systemtap-sdt-devel \
-wireshark-devel \
-yajl-devel
-rm -f /usr/lib*/python3*/EXTERNALLY-MANAGED
-rpm -qa | sort > /packages.txt
-mkdir -p /usr/libexec/ccache-wrappers
-ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc
-ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang
-ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/gcc
-/usr/bin/pip3 install black
-}
-
-export CCACHE_WRAPPERSDIR="/usr/libexec/ccache-wrappers"
-export LANG="en_US.UTF-8"
-export MAKE="/usr/bin/make"
-export NINJA="/usr/bin/ninja"
-export PYTHON="/usr/bin/python3"
diff --git a/ci/containers/centos-stream-8.Dockerfile 
b/ci/containers/centos-stream-8.Dockerfile
deleted file mode 100644
index 5765c6dfc2..00
--- a/ci/containers/centos-stream-8.Dockerfile
+++ /dev/null
@@ -1,107 +0,0 @@
-# THIS FILE WAS AUTO-GENERATED
-#
-#  $ lcitool manifest ci/manifest.yml
-#
-# https://gitlab.com/libvirt/libvirt-ci
-
-FROM quay.io/centos/centos:stream8
-
-RUN dnf distro-sync -y && \
-dnf install 'dnf-command(config-manager)' -y && \
-dnf config-manager --set-enabled -y powertools && \
-dnf install -y centos-release-advanced-virtualization && \
-dnf install -y epel-release && \
-dnf install -y epel-next-release && \
-dnf install -y \
-audit-libs-devel \
-augeas \
-bash-completion \
-ca-certificates \
-ccache \
-clang \
-cpp \
-cyrus-sasl-devel \
-device-mapper-devel \
-diffutils \
-dwarves \
-ebtables \
-firewalld-filesystem \
-fuse-devel \
-gcc \
-gettext \
-git \
-glib2-devel \
-glibc-devel \
-glibc-langpack-en \
-glusterfs-api-devel \
-gnutls-devel \
-grep \
-iproute \
-iproute-tc \
-iptables \
-iscsi-initiator-utils \
-kmod \
-libacl-devel \
-libattr-devel \
-  

Re: [PATCH 0/2] ci : drop CentOS 8 Stream

2024-04-25 Thread Michal Prívozník
On 4/25/24 15:46, Daniel P. Berrangé wrote:
> CentOS 8 Stream goes EOL at the end of May, and unlike most other
> distros, it will actively break the ability to install and update
> it, as package repos are all archived. We thus have to stop using
> it in CI, but fortunately Alma Linux 8 continues to give us enough
> ongoing coverage.
> 
> The exception is the integration tests. To keep those working would
> require the custom runner to be reinstalled with Alma Linux, which
> I don't have access to do.
> 
> Daniel P. Berrangé (2):
>   ci: refresh with latest lcitool manifest
>   ci: drop CentOS 8 Stream and refresh
> 
>  ci/buildenv/alpine-319.sh|   2 +-
>  ci/buildenv/alpine-edge.sh   |   2 +-
>  ci/buildenv/centos-stream-8.sh   | 104 -
>  ci/buildenv/centos-stream-9.sh   |   1 +
>  ci/cirrus/build.yml  |   2 +-
>  ci/containers/alpine-319.Dockerfile  |   2 +-
>  ci/containers/alpine-edge.Dockerfile |   2 +-
>  ci/containers/centos-stream-8.Dockerfile | 107 -
>  ci/containers/centos-stream-9.Dockerfile |   1 +
>  ci/gitlab/build-templates.yml| 184 ++-
>  ci/gitlab/builds.yml |  15 --
>  ci/gitlab/containers.yml |   7 -
>  ci/integration.yml   |  24 ---
>  ci/manifest.yml  |   8 -
>  14 files changed, 90 insertions(+), 371 deletions(-)
>  delete mode 100644 ci/buildenv/centos-stream-8.sh
>  delete mode 100644 ci/containers/centos-stream-8.Dockerfile
> 

Reviewed-by: Michal Privoznik 

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v3 00/27] [PATCH v3 00/27] native support for nftables in virtual network driver

2024-04-25 Thread Laine Stump
*sigh*. After posting these last night, I checked on my CI pipeline and 
found that everything had failed due to


 #if defined(FIREWALL_BACKEND_DEFAULT_IPTABLES)
  [...]
 #elif defined (FIREWALL_BACKEND_DEFAULT_IPTABLES)
 [...]
 #else
 #error blah blah
 #endif

I've pushed fixed patches to gitlab, where you can find them at:

  https://gitlab.com/lainestump/libvirt/tree/nftrereboot-7

or alternately, just change the 2nd "IPTABLES" above to "NFTABLES" in 
patch 24/27.

w
On 4/25/24 1:38 AM, Laine Stump wrote:

V2: 
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/5RTZ6PC3N3CO6X353QUHLVOL43SWQ4JD/

This patch series enables libvirt to use nftables rules rather than
iptables *when setting up virtual networks* (it does *not* add
nftables support to the nwfilter driver).

I've added the Reviewed-by's from Daniel where given in V2 (as long as
I haven't made any non-trivial changes). That includes patches 1-9,
11-13, 16, 20, and 22.

Changes from V2 - mainly I've addressed the issues that Dan pointed
out in his reviews (details in each patch). Functionally the main changes are:

1) You can now choose whether iptables or nftables should be the
default backend with the new meson option "firewall_backend" (which
is set to "nftables" by default).

2) rpm spec now requires iptables or nftables (rather than
recommending both)

3) The  element in the network status XML now has a
"name='fwRemoval'" attribute, just in case we ever add another
 element to keep track of all the commands that were run
to create the firewall as well as the commands needed to remove it.

4) Failure to find the binary needed for any firewall backend now
results in an error log and termination of the daemon.

Laine Stump (27):
   util/network: move viriptables.[ch] from util to network directory
   network: move all functions manipulating iptables rules into
 network_iptables.c
   network: make all iptables functions used only in network_iptables.c
 static
   util: #define the names used for private packet filter chains
   util: change name of virFirewallRule to virFirewallCmd
   util: rename virNetFilterAction to iptablesAction, and add
 VIR_ENUM_DECL/IMPL
   util: check for 0 args when applying iptables rule
   util: add -w/--concurrent when applying a FirewallCmd rather than when
 building it
   util: determine ignoreErrors value when creating virFirewallCmd, not
 when applying
   util/network: new virFirewallBackend enum
   network: add (empty) network.conf file to distribution files
   network: support setting firewallBackend from network.conf
   network: framework to call backend-specific function to init private
 filter chains
   util: new functions to support adding individual firewall rollback
 commands
   util: implement rollback rule autocreation for iptables commands
   network: turn on auto-rollback for the rules added for virtual
 networks
   util: add name attribute to virFirewall
   util: new function virFirewallNewFromRollback()
   util: new functions virFirewallParseXML() and virFirewallFormat()
   conf: add a virFirewall object to virNetworkObj
   network: use previously saved list of firewall removal commands
   network: save network status when firewall rules are reloaded
   meson: stop looking for iptables/ip6tables/ebtables at build time
   network: add an nftables backend for network driver's firewall
 construction
   tests: test cases for nftables backend
   network: prefer the nftables backend over iptables
   spec: require either iptables or nftables if network driver is
 installed

  libvirt.spec.in   |7 +-
  meson.build   |   10 +-
  meson_options.txt |1 +
  po/POTFILES   |3 +-
  src/conf/virnetworkobj.c  |   47 +
  src/conf/virnetworkobj.h  |   11 +
  src/libvirt_private.syms  |   59 +-
  src/network/bridge_driver.c   |   35 +-
  src/network/bridge_driver_conf.c  |   64 +
  src/network/bridge_driver_conf.h  |3 +
  src/network/bridge_driver_linux.c |  630 +--
  src/network/bridge_driver_nop.c   |6 +-
  src/network/bridge_driver_platform.h  |6 +-
  src/network/libvirtd_network.aug  |   39 +
  src/network/meson.build   |   36 +
  src/network/network.conf.in   |   28 +
  src/network/network_iptables.c| 1677 +
  src/network/network_iptables.h|   30 +
  src/network/network_nftables.c|  940 +
  src/network/network_nftables.h|   28 +
  src/network/test_libvirtd_network.aug.in  |5 +
  src/nwfilter/nwfilter_ebiptables_driver.c | 1004 +-
  src/util/meson.build  |1 -
  src/util/vireb

Re: [PATCH 0/2] ci : drop CentOS 8 Stream

2024-04-25 Thread Daniel P . Berrangé
On Thu, Apr 25, 2024 at 03:55:54PM +0200, Michal Prívozník wrote:
> On 4/25/24 15:46, Daniel P. Berrangé wrote:
> > CentOS 8 Stream goes EOL at the end of May, and unlike most other
> > distros, it will actively break the ability to install and update
> > it, as package repos are all archived. We thus have to stop using
> > it in CI, but fortunately Alma Linux 8 continues to give us enough
> > ongoing coverage.
> > 
> > The exception is the integration tests. To keep those working would
> > require the custom runner to be reinstalled with Alma Linux, which
> > I don't have access to do.
> > 
> > Daniel P. Berrangé (2):
> >   ci: refresh with latest lcitool manifest
> >   ci: drop CentOS 8 Stream and refresh
> > 
> >  ci/buildenv/alpine-319.sh|   2 +-
> >  ci/buildenv/alpine-edge.sh   |   2 +-
> >  ci/buildenv/centos-stream-8.sh   | 104 -
> >  ci/buildenv/centos-stream-9.sh   |   1 +
> >  ci/cirrus/build.yml  |   2 +-
> >  ci/containers/alpine-319.Dockerfile  |   2 +-
> >  ci/containers/alpine-edge.Dockerfile |   2 +-
> >  ci/containers/centos-stream-8.Dockerfile | 107 -
> >  ci/containers/centos-stream-9.Dockerfile |   1 +
> >  ci/gitlab/build-templates.yml| 184 ++-
> >  ci/gitlab/builds.yml |  15 --
> >  ci/gitlab/containers.yml |   7 -
> >  ci/integration.yml   |  24 ---
> >  ci/manifest.yml  |   8 -
> >  14 files changed, 90 insertions(+), 371 deletions(-)
> >  delete mode 100644 ci/buildenv/centos-stream-8.sh
> >  delete mode 100644 ci/containers/centos-stream-8.Dockerfile
> > 
> 
> Reviewed-by: Michal Privoznik 

BTW, I'lll hold this until after the release.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] vmx: Check serialX.vspc before serialX.fileName

2024-04-25 Thread Michal Prívozník
On 4/25/24 14:12, Martin Kletzander wrote:
> When using vSPC (Virtual Serial Port Concentrator) in vSphere the actual
> address for it is saved in serialX.vspc in which case the
> serialX.fileName is most probably something we can't get any useful
> information from and we also fail during the parsing rendering any
> dumpxml and similar tries unsuccessful.
> 
> Instead of parsing the vspc URL with something along the lines of
> `virURIParse(vspc ? vspc : fileName)`, which could lead to us reporting
> information that is very prune to misuse (the vSPC seemingly has a
> protocol on top of the telnet connection; redefining the domain would
> change the behaviour; the URL might have a fragment we are not saving;
> etc.) or adding more XML knobs to indicate vSPC usage (which we would
> not be able to configure; we'd have to properly error out everywhere;
> etc.) let's just report dummy serial port that leads to nowhere.
> 
> Resolves: https://issues.redhat.com/browse/RHEL-32182
> Signed-off-by: Martin Kletzander 
> ---
>  src/vmx/vmx.c| 12 +++
>  tests/vmx2xmldata/esx-in-the-wild-13.vmx | 97 
>  tests/vmx2xmldata/esx-in-the-wild-13.xml | 55 ++
>  tests/vmx2xmltest.c  |  1 +
>  4 files changed, 165 insertions(+)
>  create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.vmx
>  create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.xml

Reviewed-by: Michal Privoznik 

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 0/6] migration removals & deprecations

2024-04-25 Thread Fabiano Rosas
Hi everyone,

Here's some cleaning up of deprecated code. It removes the old block
migration and compression code. Both have suitable replacements in the
form of the blockdev-mirror driver and multifd compression,
respectively.

There's also a deprecation for fd: + file to cope with the fact that
the new MigrationAddress API defines transports instead of protocols
(loose terms) like the old string API did. So we cannot map 1:1 from
fd: to any transport because fd: allows *both* file migration and
socket migration.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1267859704

Fabiano Rosas (6):
  migration: Remove 'skipped' field from MigrationStats
  migration: Remove 'inc' option from migrate command
  migration: Remove 'blk/-b' option from migrate commands
  migration: Remove block migration
  migration: Remove non-multifd compression
  migration: Deprecate fd: for file migration

 .gitlab-ci.d/buildtest.yml   |2 +-
 MAINTAINERS  |1 -
 docs/about/deprecated.rst|   51 +-
 docs/about/removed-features.rst  |  104 ++-
 docs/devel/migration/main.rst|2 +-
 hw/core/machine.c|1 -
 include/migration/misc.h |6 -
 meson.build  |2 -
 meson_options.txt|2 -
 migration/block.c| 1019 --
 migration/block.h|   52 --
 migration/colo.c |1 -
 migration/meson.build|4 -
 migration/migration-hmp-cmds.c   |   97 +--
 migration/migration.c|   70 +-
 migration/migration.h|7 -
 migration/options.c  |  229 ---
 migration/ram-compress.c |  564 -
 migration/ram.c  |  166 +
 migration/savevm.c   |5 -
 qapi/migration.json  |  205 +-
 scripts/meson-buildoptions.sh|4 -
 tests/qemu-iotests/183   |  147 -
 tests/qemu-iotests/183.out   |   66 --
 tests/qemu-iotests/common.filter |7 -
 tests/qtest/migration-test.c |  139 
 26 files changed, 130 insertions(+), 2823 deletions(-)
 delete mode 100644 migration/block.c
 delete mode 100644 migration/block.h
 delete mode 100644 migration/ram-compress.c
 delete mode 100755 tests/qemu-iotests/183
 delete mode 100644 tests/qemu-iotests/183.out

-- 
2.35.3
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 1/6] migration: Remove 'skipped' field from MigrationStats

2024-04-25 Thread Fabiano Rosas
The 'skipped' field of the MigrationStats struct has been deprecated
in 8.1. Time to remove it.

Deprecation commit 7b24d32634 ("migration: skipped field is really
obsolete.").

Signed-off-by: Fabiano Rosas 
---
 docs/about/deprecated.rst   | 6 --
 docs/about/removed-features.rst | 6 ++
 migration/migration-hmp-cmds.c  | 2 --
 migration/migration.c   | 2 --
 qapi/migration.json | 8 
 5 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7b548519b5..4d9d6bf2da 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -488,12 +488,6 @@ option).
 Migration
 -
 
-``skipped`` MigrationStats field (since 8.1)
-
-
-``skipped`` field in Migration stats has been deprecated.  It hasn't
-been used for more than 10 years.
-
 ``inc`` migrate command option (since 8.2)
 ''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index f9cf874f7b..9873f59bee 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -614,6 +614,12 @@ was superseded by ``sections``.
 Member ``section-size`` in the return value of ``query-sgx-capabilities``
 was superseded by ``sections``.
 
+``query-migrate`` return value member ``skipped`` (removed in 9.1)
+''
+
+Member ``skipped`` of the ``MigrationStats`` struct hasn't been used
+for more than 10 years. Removed with no replacement.
+
 Human Monitor Protocol (HMP) commands
 -
 
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 7e96ae6ffd..28f776d06d 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -105,8 +105,6 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
info->ram->total >> 10);
 monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
info->ram->duplicate);
-monitor_printf(mon, "skipped: %" PRIu64 " pages\n",
-   info->ram->skipped);
 monitor_printf(mon, "normal: %" PRIu64 " pages\n",
info->ram->normal);
 monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
diff --git a/migration/migration.c b/migration/migration.c
index 696762bc64..3b433fdb31 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1149,8 +1149,6 @@ static void populate_ram_info(MigrationInfo *info, 
MigrationState *s)
 info->ram->transferred = migration_transferred_bytes();
 info->ram->total = ram_bytes_total();
 info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
-/* legacy value.  It is not used anymore */
-info->ram->skipped = 0;
 info->ram->normal = stat64_get(&mig_stats.normal_pages);
 info->ram->normal_bytes = info->ram->normal * page_size;
 info->ram->mbps = s->mbps;
diff --git a/qapi/migration.json b/qapi/migration.json
index 8c65b90328..401b8e24ac 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -23,9 +23,6 @@
 #
 # @duplicate: number of duplicate (zero) pages (since 1.2)
 #
-# @skipped: number of skipped zero pages.  Always zero, only provided
-# for compatibility (since 1.5)
-#
 # @normal: number of normal pages (since 1.2)
 #
 # @normal-bytes: number of normal bytes sent (since 1.2)
@@ -63,16 +60,11 @@
 # between 0 and @dirty-sync-count * @multifd-channels.  (since
 # 7.1)
 #
-# Features:
-#
-# @deprecated: Member @skipped is always zero since 1.5.3
-#
 # Since: 0.14
 ##
 { 'struct': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
'duplicate': 'int',
-   'skipped': { 'type': 'int', 'features': [ 'deprecated' ] },
'normal': 'int',
'normal-bytes': 'int', 'dirty-pages-rate': 'int',
'mbps': 'number', 'dirty-sync-count': 'int',
-- 
2.35.3
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 2/6] migration: Remove 'inc' option from migrate command

2024-04-25 Thread Fabiano Rosas
The block incremental option for block migration has been deprecated
in 8.2 in favor of using the block-mirror feature. Remove it now.

Deprecation commit 40101f320d ("migration: migrate 'inc' command
option is deprecated.").

Signed-off-by: Fabiano Rosas 
---
 docs/about/deprecated.rst   |  9 --
 docs/about/removed-features.rst | 14 +
 migration/block.c   |  1 -
 migration/migration-hmp-cmds.c  | 18 ++-
 migration/migration.c   | 24 +--
 migration/options.c | 30 +-
 qapi/migration.json | 54 +++--
 7 files changed, 35 insertions(+), 115 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 4d9d6bf2da..25b309dde4 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -488,15 +488,6 @@ option).
 Migration
 -
 
-``inc`` migrate command option (since 8.2)
-''
-
-Use blockdev-mirror with NBD instead.
-
-As an intermediate step the ``inc`` functionality can be achieved by
-setting the ``block-incremental`` migration parameter to ``true``.
-But this parameter is also deprecated.
-
 ``blk`` migrate command option (since 8.2)
 ''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 9873f59bee..e62fc760f1 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -620,6 +620,13 @@ was superseded by ``sections``.
 Member ``skipped`` of the ``MigrationStats`` struct hasn't been used
 for more than 10 years. Removed with no replacement.
 
+``migrate`` command option ``inc`` (removed in 9.1)
+'''
+
+Use blockdev-mirror with NBD instead. See "QMP invocation for live
+storage migration with ``blockdev-mirror`` + NBD" in
+docs/interop/live-block-operations.rst for a detailed explanation.
+
 Human Monitor Protocol (HMP) commands
 -
 
@@ -680,6 +687,13 @@ This command didn't produce any output already. Removed 
with no replacement.
 The ``singlestep`` command has been replaced by the ``one-insn-per-tb``
 command, which has the same behaviour but a less misleading name.
 
+``migrate`` command ``-i`` option (removed in 9.1)
+''
+
+Use blockdev-mirror with NBD instead. See "QMP invocation for live
+storage migration with ``blockdev-mirror`` + NBD" in
+docs/interop/live-block-operations.rst for a detailed explanation.
+
 Host Architectures
 --
 
diff --git a/migration/block.c b/migration/block.c
index bae6e94891..87ec1a7e68 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -419,7 +419,6 @@ static int init_blk_migration(QEMUFile *f, Error **errp)
 bmds->bulk_completed = 0;
 bmds->total_sectors = sectors;
 bmds->completed_sectors = 0;
-bmds->shared_base = migrate_block_incremental();
 
 assert(i < num_bs);
 bmds_bs[i].bmds = bmds;
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 28f776d06d..f49f061be1 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -332,10 +332,6 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict 
*qdict)
 monitor_printf(mon, "%s: %u ms\n",
 MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
 params->x_checkpoint_delay);
-assert(params->has_block_incremental);
-monitor_printf(mon, "%s: %s\n",
-MigrationParameter_str(MIGRATION_PARAMETER_BLOCK_INCREMENTAL),
-params->block_incremental ? "on" : "off");
 monitor_printf(mon, "%s: %u\n",
 MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS),
 params->multifd_channels);
@@ -616,10 +612,6 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
*qdict)
 p->has_x_checkpoint_delay = true;
 visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
 break;
-case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
-p->has_block_incremental = true;
-visit_type_bool(v, param, &p->block_incremental, &err);
-break;
 case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
 p->has_multifd_channels = true;
 visit_type_uint8(v, param, &p->multifd_channels, &err);
@@ -767,18 +759,12 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 {
 bool detach = qdict_get_try_bool(qdict, "detach", false);
 bool blk = qdict_get_try_bool(qdict, "blk", false);
-bool inc = qdict_get_try_bool(qdict, "inc", false);
 bool resume = qdict_get_try_bool(qdict, "resume", false);
 const char *uri = qdict_get_str(qdict, "uri");
 Error *err = NULL;
 g_autoptr(MigrationChannelList) caps = NULL;
 g_autoptr(MigrationChannel) channel = NULL;
 
-if (inc) {
-warn_report("option '-i' is deprecated;"
-  

[PATCH 3/6] migration: Remove 'blk/-b' option from migrate commands

2024-04-25 Thread Fabiano Rosas
The block migration is considered obsolete and has been deprecated in
8.2. Remove the migrate command option that enables it. This only
affects the QMP and HMP commands, the feature can still be accessed by
setting the migration 'block' capability. The whole feature will be
removed in a future patch.

Deprecation commit 8846b5bfca ("migration: migrate 'blk' command
option is deprecated.").

Signed-off-by: Fabiano Rosas 
---
 .gitlab-ci.d/buildtest.yml   |   2 +-
 docs/about/deprecated.rst|   9 --
 docs/about/removed-features.rst  |  15 +++-
 migration/migration-hmp-cmds.c   |   9 +-
 migration/migration.c|  31 ++-
 migration/options.c  |   3 +-
 qapi/migration.json  |   8 --
 tests/qemu-iotests/183   | 147 ---
 tests/qemu-iotests/183.out   |  66 --
 tests/qemu-iotests/common.filter |   7 --
 10 files changed, 22 insertions(+), 275 deletions(-)
 delete mode 100755 tests/qemu-iotests/183
 delete mode 100644 tests/qemu-iotests/183.out

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index cfdff175c3..b1a536592f 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -342,7 +342,7 @@ build-tcg-disabled:
 - cd tests/qemu-iotests/
 - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048
 052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163
-170 171 183 184 192 194 208 221 226 227 236 253 277 image-fleecing
+170 171 184 192 194 208 221 226 227 236 253 277 image-fleecing
 - ./check -qcow2 028 051 056 057 058 065 068 082 085 091 095 096 102 122
 124 132 139 142 144 145 151 152 155 157 165 194 196 200 202
 208 209 216 218 227 234 246 247 248 250 254 255 257 258
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 25b309dde4..83adb763e6 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -488,15 +488,6 @@ option).
 Migration
 -
 
-``blk`` migrate command option (since 8.2)
-''
-
-Use blockdev-mirror with NBD instead.
-
-As an intermediate step the ``blk`` functionality can be achieved by
-setting the ``block`` migration capability to ``true``.  But this
-capability is also deprecated.
-
 block migration (since 8.2)
 '''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index e62fc760f1..0ba94c826a 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -627,6 +627,13 @@ Use blockdev-mirror with NBD instead. See "QMP invocation 
for live
 storage migration with ``blockdev-mirror`` + NBD" in
 docs/interop/live-block-operations.rst for a detailed explanation.
 
+``migrate`` command option ``blk`` (removed in 9.1)
+'''
+
+Use blockdev-mirror with NBD instead. See "QMP invocation for live
+storage migration with ``blockdev-mirror`` + NBD" in
+docs/interop/live-block-operations.rst for a detailed explanation.
+
 Human Monitor Protocol (HMP) commands
 -
 
@@ -694,6 +701,13 @@ Use blockdev-mirror with NBD instead. See "QMP invocation 
for live
 storage migration with ``blockdev-mirror`` + NBD" in
 docs/interop/live-block-operations.rst for a detailed explanation.
 
+``migrate`` command ``-b`` option (removed in 9.1)
+''
+
+Use blockdev-mirror with NBD instead. See "QMP invocation for live
+storage migration with ``blockdev-mirror`` + NBD" in
+docs/interop/live-block-operations.rst for a detailed explanation.
+
 Host Architectures
 --
 
@@ -1025,4 +1039,3 @@ There is a newer Rust implementation of ``virtiofsd`` at
 stable for some time and is now widely used.
 The command line and feature set is very close to the removed
 C implementation.
-
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index f49f061be1..5ab204d91d 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -758,26 +758,19 @@ static void hmp_migrate_status_cb(void *opaque)
 void hmp_migrate(Monitor *mon, const QDict *qdict)
 {
 bool detach = qdict_get_try_bool(qdict, "detach", false);
-bool blk = qdict_get_try_bool(qdict, "blk", false);
 bool resume = qdict_get_try_bool(qdict, "resume", false);
 const char *uri = qdict_get_str(qdict, "uri");
 Error *err = NULL;
 g_autoptr(MigrationChannelList) caps = NULL;
 g_autoptr(MigrationChannel) channel = NULL;
 
-if (blk) {
-warn_report("option '-b' is deprecated;"
-" use blockdev-mirror with NBD instead");
-}
-
 if (!migrate_uri_parse(uri, &channel, &err)) {
 hmp_handle_error(mon, err);
 return;
 }
 QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
 
-qmp_migrate(NULL, true, caps, !!blk, blk, false, false,
- 

[PATCH 6/6] migration: Deprecate fd: for file migration

2024-04-25 Thread Fabiano Rosas
The fd: URI can currently trigger two different types of migration, a
TCP migration using sockets and a file migration using a plain
file. This is in conflict with the recently introduced (8.2) QMP
migrate API that takes structured data as JSON-like format. We cannot
keep the same backend for both types of migration because with the new
API the code is more tightly coupled to the type of transport. This
means a TCP migration must use the 'socket' transport and a file
migration must use the 'file' transport.

If we keep allowing fd: when using a file, this creates an issue when
the user converts the old-style (fd:) to the new style ("transport":
"socket") invocation because the file descriptor in question has
previously been allowed to be either a plain file or a socket.

To avoid creating too much confusion, we can simply deprecate the fd:
+ file usage, which is thought to be rarely used currently and instead
establish a 1:1 correspondence between fd: URI and socket transport,
and file: URI and file transport.

Signed-off-by: Fabiano Rosas 
---
 docs/about/deprecated.rst | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index fadb85f289..11dce324c8 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -484,3 +484,17 @@ both, older and future versions of QEMU.
 The ``blacklist`` config file option has been renamed to ``block-rpcs``
 (to be in sync with the renaming of the corresponding command line
 option).
+
+Migration
+-
+
+``fd:`` URI when used for file migration (since 9.1)
+
+
+The ``fd:`` URI can currently provide a file descriptor that
+references either a socket or a plain file. These are two different
+types of migration. In order to reduce ambiguity, the ``fd:`` URI
+usage of providing a file descriptor to a plain file has been
+deprecated in favor of explicitly using the ``file:`` URI with the
+file descriptor being passed as an ``fdset``. Refer to the ``add-fd``
+command documentation for details on the ``fdset`` usage.
-- 
2.35.3
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 4/6] migration: Remove block migration

2024-04-25 Thread Fabiano Rosas
The block migration has been considered obsolete since QEMU 8.2 in
favor of the more flexible storage migration provided by the
blockdev-mirror driver. Two releases have passed so now it's time to
remove it.

Deprecation commit 66db46ca83 ("migration: Deprecate block
migration").

Signed-off-by: Fabiano Rosas 
---
 MAINTAINERS |1 -
 docs/about/deprecated.rst   |   10 -
 docs/about/removed-features.rst |   14 +
 docs/devel/migration/main.rst   |2 +-
 include/migration/misc.h|6 -
 meson.build |2 -
 meson_options.txt   |2 -
 migration/block.c   | 1018 ---
 migration/block.h   |   52 --
 migration/colo.c|1 -
 migration/meson.build   |3 -
 migration/migration-hmp-cmds.c  |   25 -
 migration/migration.c   |   17 -
 migration/options.c |   36 --
 migration/ram.c |   15 -
 migration/savevm.c  |5 -
 qapi/migration.json |   61 +-
 scripts/meson-buildoptions.sh   |4 -
 18 files changed, 26 insertions(+), 1248 deletions(-)
 delete mode 100644 migration/block.c
 delete mode 100644 migration/block.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f1f6922025..9a53ac9ec2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2877,7 +2877,6 @@ F: util/aio-*.h
 F: util/defer-call.c
 F: util/fdmon-*.c
 F: block/io.c
-F: migration/block*
 F: include/block/aio.h
 F: include/block/aio-wait.h
 F: include/qemu/defer-call.h
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 83adb763e6..409c9e0c4d 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -488,16 +488,6 @@ option).
 Migration
 -
 
-block migration (since 8.2)
-'''
-
-Block migration is too inflexible.  It needs to migrate all block
-devices or none.
-
-Please see "QMP invocation for live storage migration with
-``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
-for a detailed explanation.
-
 old compression method (since 8.2)
 ''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 0ba94c826a..dd20f37f2c 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -634,6 +634,13 @@ Use blockdev-mirror with NBD instead. See "QMP invocation 
for live
 storage migration with ``blockdev-mirror`` + NBD" in
 docs/interop/live-block-operations.rst for a detailed explanation.
 
+``migrate-set-capabilities`` ``block`` option (removed in 9.1)
+''
+
+Block migration has been removed. For a replacement, see "QMP
+invocation for live storage migration with ``blockdev-mirror`` + NBD"
+in docs/interop/live-block-operations.rst.
+
 Human Monitor Protocol (HMP) commands
 -
 
@@ -708,6 +715,13 @@ Use blockdev-mirror with NBD instead. See "QMP invocation 
for live
 storage migration with ``blockdev-mirror`` + NBD" in
 docs/interop/live-block-operations.rst for a detailed explanation.
 
+``migrate_set_capability`` ``block`` option (removed in 9.1)
+
+
+Block migration has been removed. For a replacement, see "QMP
+invocation for live storage migration with ``blockdev-mirror`` + NBD"
+in docs/interop/live-block-operations.rst.
+
 Host Architectures
 --
 
diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 54385a23e5..495cdcb112 100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -454,7 +454,7 @@ Examples of such API functions are:
 Iterative device migration
 --
 
-Some devices, such as RAM, Block storage or certain platform devices,
+Some devices, such as RAM or certain platform devices,
 have large amounts of data that would mean that the CPUs would be
 paused for too long if they were sent in one section.  For these
 devices an *iterative* approach is taken.
diff --git a/include/migration/misc.h b/include/migration/misc.h
index c9e200f4eb..bf7339cc1e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -45,12 +45,6 @@ bool migrate_ram_is_ignored(RAMBlock *block);
 
 /* migration/block.c */
 
-#ifdef CONFIG_LIVE_BLOCK_MIGRATION
-void blk_mig_init(void);
-#else
-static inline void blk_mig_init(void) {}
-#endif
-
 AnnounceParameters *migrate_announce_params(void);
 /* migration/savevm.c */
 
diff --git a/meson.build b/meson.build
index 84e59dcbb4..2096050fd2 100644
--- a/meson.build
+++ b/meson.build
@@ -2350,7 +2350,6 @@ config_host_data.set('CONFIG_DEBUG_GRAPH_LOCK', 
get_option('debug_graph_lock'))
 config_host_data.set('CONFIG_DEBUG_MUTEX', get_option('debug_mutex'))
 config_host_data.set('CONFIG_DEBUG_STACK_USAGE', 
get_option('debug_stack_usage'))
 config_host_data.set('CONFIG_DEBUG_TCG', get_option('debug_tcg'))

[PATCH 5/6] migration: Remove non-multifd compression

2024-04-25 Thread Fabiano Rosas
The 'compress' migration capability enables the old compression code
which has shown issues over the years and is thought to be less stable
and tested than the more recent multifd-based compression. The old
compression code has been deprecated in 8.2 and now is time to remove
it.

Deprecation commit 864128df46 ("migration: Deprecate old compression
method").

Signed-off-by: Fabiano Rosas 
---
 docs/about/deprecated.rst   |  11 -
 docs/about/removed-features.rst |  55 
 hw/core/machine.c   |   1 -
 migration/meson.build   |   1 -
 migration/migration-hmp-cmds.c  |  47 ---
 migration/migration.c   |  14 -
 migration/migration.h   |   7 -
 migration/options.c | 164 --
 migration/ram-compress.c| 564 
 migration/ram.c | 151 +
 qapi/migration.json | 112 ---
 tests/qtest/migration-test.c| 139 
 12 files changed, 63 insertions(+), 1203 deletions(-)
 delete mode 100644 migration/ram-compress.c

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 409c9e0c4d..fadb85f289 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -484,14 +484,3 @@ both, older and future versions of QEMU.
 The ``blacklist`` config file option has been renamed to ``block-rpcs``
 (to be in sync with the renaming of the corresponding command line
 option).
-
-Migration
--
-
-old compression method (since 8.2)
-''
-
-Compression method fails too much.  Too many races.  We are going to
-remove it if nobody fixes it.  For starters, migration-test
-compression tests are disabled because they fail randomly.  If you need
-compression, use multifd compression methods.
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index dd20f37f2c..783f6b479d 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -505,6 +505,11 @@ configurations (e.g. -smp 8,sockets=0) is removed since 
9.0, users have
 to ensure that all the topology members described with -smp are greater
 than zero.
 
+``-global migration.decompress-error-check`` (removed in 9.1)
+'
+
+Removed along with the ``compression`` migration capability.
+
 User-mode emulator command line arguments
 -
 
@@ -641,6 +646,31 @@ Block migration has been removed. For a replacement, see 
"QMP
 invocation for live storage migration with ``blockdev-mirror`` + NBD"
 in docs/interop/live-block-operations.rst.
 
+``migrate-set-parameter`` ``compress-level`` option (removed in 9.1)
+
+
+Use ``multifd-zlib-level`` or ``multifd-zstd-level`` instead.
+
+``migrate-set-parameter`` ``compress-threads`` option (removed in 9.1)
+''
+
+Use ``multifd-channels`` instead.
+
+``migrate-set-parameter`` ``compress-wait-thread`` option (removed in 9.1)
+''
+
+Removed with no replacement.
+
+``migrate-set-parameter`` ``decompress-threads`` option (removed in 9.1)
+
+
+Use ``multifd-channels`` instead.
+
+``migrate-set-capability`` ``compress`` option (removed in 9.1)
+'''
+
+Use ``multifd-compression`` instead.
+
 Human Monitor Protocol (HMP) commands
 -
 
@@ -722,6 +752,31 @@ Block migration has been removed. For a replacement, see 
"QMP
 invocation for live storage migration with ``blockdev-mirror`` + NBD"
 in docs/interop/live-block-operations.rst.
 
+``migrate_set_parameter`` ``compress-level`` option (removed in 9.1)
+
+
+Use ``multifd-zlib-level`` or ``multifd-zstd-level`` instead.
+
+``migrate_set_parameter`` ``compress-threads`` option (removed in 9.1)
+''
+
+Use ``multifd-channels`` instead.
+
+``migrate_set_parameter`` ``compress-wait-thread`` option (removed in 9.1)
+''
+
+Removed with no replacement.
+
+``migrate_set_parameter`` ``decompress-threads`` option (removed in 9.1)
+
+
+Use ``multifd-channels`` instead.
+
+``migrate_set_capability`` ``compress`` option (removed in 9.1)
+'''
+
+Use ``multifd-compression`` instead.
+
 Host Architectures
 --
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 582c2df37a..596ff77e3c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -190,7 +190,6 @@ GlobalProper

Re: [PATCH] vmx: Check serialX.vspc before serialX.fileName

2024-04-25 Thread Peter Krempa
On Thu, Apr 25, 2024 at 14:12:54 +0200, Martin Kletzander wrote:
> When using vSPC (Virtual Serial Port Concentrator) in vSphere the actual
> address for it is saved in serialX.vspc in which case the
> serialX.fileName is most probably something we can't get any useful
> information from and we also fail during the parsing rendering any
> dumpxml and similar tries unsuccessful.
> 
> Instead of parsing the vspc URL with something along the lines of
> `virURIParse(vspc ? vspc : fileName)`, which could lead to us reporting
> information that is very prune to misuse (the vSPC seemingly has a
> protocol on top of the telnet connection; redefining the domain would
> change the behaviour; the URL might have a fragment we are not saving;
> etc.) or adding more XML knobs to indicate vSPC usage (which we would
> not be able to configure; we'd have to properly error out everywhere;
> etc.) let's just report dummy serial port that leads to nowhere.
> 
> Resolves: https://issues.redhat.com/browse/RHEL-32182
> Signed-off-by: Martin Kletzander 
> ---
>  src/vmx/vmx.c| 12 +++
>  tests/vmx2xmldata/esx-in-the-wild-13.vmx | 97 
>  tests/vmx2xmldata/esx-in-the-wild-13.xml | 55 ++
>  tests/vmx2xmltest.c  |  1 +
>  4 files changed, 165 insertions(+)
>  create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.vmx
>  create mode 100644 tests/vmx2xmldata/esx-in-the-wild-13.xml
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 5da67aae60d9..32074f62e14c 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -2975,6 +2975,9 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, 
> int port,
>  char fileName_name[48] = "";
>  g_autofree char *fileName = NULL;
>  
> +char vspc_name[48] = "";
> +g_autofree char *vspc = NULL;
> +
>  char network_endPoint_name[48] = "";
>  g_autofree char *network_endPoint = NULL;
>  
> @@ -2997,6 +3000,7 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, 
> int port,
>  VMX_BUILD_NAME(startConnected);
>  VMX_BUILD_NAME(fileType);
>  VMX_BUILD_NAME(fileName);
> +VMX_BUILD_NAME(vspc);
>  VMX_BUILD_NAME_EXTRA(network_endPoint, "network.endPoint");
>  
>  /* vmx:present */
> @@ -3026,6 +3030,10 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, 
> int port,
>  if (virVMXGetConfigString(conf, fileName_name, &fileName, true) < 0)
>  goto cleanup;
>  
> +/* vmx:fileName -> def:data.file.path */
> +if (virVMXGetConfigString(conf, vspc_name, &vspc, true) < 0)
> +goto cleanup;
> +
>  /* vmx:network.endPoint -> def:data.tcp.listen */
>  if (virVMXGetConfigString(conf, network_endPoint_name, &network_endPoint,
>true) < 0) {
> @@ -3057,6 +3065,10 @@ virVMXParseSerial(virVMXContext *ctx, virConf *conf, 
> int port,
>  (*def)->target.port = port;
>  (*def)->source->type = VIR_DOMAIN_CHR_TYPE_PIPE;
>  (*def)->source->data.file.path = g_steal_pointer(&fileName);
> +} else if (STRCASEEQ(fileType, "network") && vspc) {
> +(*def)->target.port = port;
> +(*def)->source->type = VIR_DOMAIN_CHR_TYPE_FILE;

Wouldn't a _TYPE_NULL be more reasonable in this case? Or is the
intention to actually have a path?
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v3 12/27] network: support setting firewallBackend from network.conf

2024-04-25 Thread Daniel P . Berrangé
On Thu, Apr 25, 2024 at 01:38:18AM -0400, Laine Stump wrote:
> It still can have only one useful value ("iptables"), but once a 2nd
> value is supported, it will be selectable by setting
> "firewall_backend=nftables" in /etc/libvirt/network.conf.
> 
> If firewall_backend isn't set in network.conf, then libvirt will check
> to see if the iptables binary is present on the system and set
> firewallBackend to iptables - if no iptables binary is found, that is
> considered a fatal error (since no networks can be started anyway), so
> an error is logged and startup of the network driver fails.
> 
> NB: network.conf is itself created from network.conf.in at build time,
> and the advertised default setting of firewall_backend (in a commented
> out line) is set from the meson_options.txt setting
> "firewall_backend". This way the conf file will have correct
> information no matter what default backend is chosen at build time.
> 
> Signed-off-by: Laine Stump 
> Reviewed-by: Daniel P. Berrangé 
> ---
> Changes from V2:
> 
>  * make failure to find iptables during network driver init fatal
> 
>  * recognize firewall_backend setting in meson_options.txt and
>propagate it through to:
> 
> * meson-config.h (along with a numeric constant
>   FIREWALL_BACKEND_DEFAULT_IPTABLES, which can be used for
>   conditional compilation)
> * network.conf - default setting and comments use @FIREWALL_BACKEND@
> * test_libvirtd_network.aug.in so that default firewall backend
>   for testing is set properly (this required calisthenics similar to
>   qemu_user_group_hack_conf in the qemu driver's meson.build
>   (see commit 64a7b8203bf)
> 
>  meson.build  |  5 +++
>  meson_options.txt|  1 +
>  src/network/bridge_driver.c  | 22 ++-
>  src/network/bridge_driver_conf.c | 50 
>  src/network/bridge_driver_conf.h |  3 ++
>  src/network/bridge_driver_linux.c|  6 ++-
>  src/network/bridge_driver_nop.c  |  6 ++-
>  src/network/bridge_driver_platform.h |  6 ++-
>  src/network/libvirtd_network.aug |  5 ++-
>  src/network/meson.build  | 22 ++-
>  src/network/network.conf.in  |  8 
>  src/network/test_libvirtd_network.aug.in |  3 ++
>  tests/networkxml2firewalltest.c  |  2 +-
>  13 files changed, 119 insertions(+), 20 deletions(-)


> diff --git a/src/network/bridge_driver_conf.c 
> b/src/network/bridge_driver_conf.c
> index a2edafa837..25cbbf8933 100644
> --- a/src/network/bridge_driver_conf.c
> +++ b/src/network/bridge_driver_conf.c
> @@ -25,6 +25,7 @@
>  #include "datatypes.h"
>  #include "virlog.h"
>  #include "virerror.h"
> +#include "virfile.h"
>  #include "virutil.h"
>  #include "bridge_driver_conf.h"
>  
> @@ -62,6 +63,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
> G_GNUC_UNUSED,
> const char *filename)
>  {
>  g_autoptr(virConf) conf = NULL;
> +g_autofree char *firewallBackendStr = NULL;
>  
>  /* if file doesn't exist or is unreadable, ignore the "error" */
>  if (access(filename, R_OK) == -1)

Not visible, but here we are about to do 'return 0'.

This means that if the config file doesn't exist at all, then
none of this method below will run.

The logic for detecting the "best" firewall backend in the
"} else {" clause below will thus never run. So without a
config file on disk, it will always implicitly get the
iptables backend set.

We should set all defaults upfront, which means detectnig
nft vs iptables. Then set an override later when reading
the config, if it exists.

> @@ -73,6 +75,54 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
> G_GNUC_UNUSED,
>  
>  /* use virConfGetValue*(conf, ...) functions to read any settings into 
> cfg */
>  
> +if (virConfGetValueString(conf, "firewall_backend", &firewallBackendStr) 
> < 0)
> +return -1;
> +
> +if (firewallBackendStr) {
> +int backend = virFirewallBackendTypeFromString(firewallBackendStr);
> +
> +if (backend < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("unknown value for 'firewall_backend' in 
> network.conf: '%1$s'"),
> +   firewallBackendStr);
> +return -1;
> +}
> +
> +cfg->firewallBackend = backend;
> +VIR_INFO("using firewall_backend setting from network.conf: '%s'",
> + virFirewallBackendTypeToString(cfg->firewallBackend));
> +
> +} else {
> +
> +/* no .conf setting, so see what this host supports by looking
> + * for binaries used by the backends, and set accordingly.
> + */
> +g_autofree char *iptablesInPath = NULL;
> +bool binaryFound = false;
> +
> +/* virFindFileInPath() uses g_find_program_in_path(),
> + * which allows absolute paths, and verifies that
> + * the 

Re: [PATCH v3 20/27] conf: add a virFirewall object to virNetworkObj

2024-04-25 Thread Daniel P . Berrangé
On Thu, Apr 25, 2024 at 01:38:26AM -0400, Laine Stump wrote:
> This virFirewall object will store the list of actions required to
> remove the firewall that was added for the currently active instance
> of the network, so it has been named "fwRemoval" (and when parsed into
> XML, the  element will have the name "fwRemoval").
> 
> There are no uses of the fwRemoval object in the virNetworkObj yet,
> but everything is in place to add it to the XML when formatted, parse
> it from the XML when reading network status, and free the virFirewall
> object when the virNetworkObj is freed.
> 
> Signed-off-by: Laine Stump 
> Reviewed-by: Daniel P. Berrangé 
> ---
> Change from V2:
> * add name='fwRemoval' to the network status  so it
>   can be positively identified in the XML
> 
>  src/conf/virnetworkobj.c | 46 
>  src/conf/virnetworkobj.h | 11 ++
>  src/libvirt_private.syms |  3 +++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index d5aa121e20..fef4c69004 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -55,6 +55,11 @@ struct _virNetworkObj {
>  
>  unsigned int taint;
>  
> +/* fwRemoval contains all commands needed to remove the firewall
> + * that was added for this network.
> + */
> +virFirewall *fwRemoval;
> +
>  /* Immutable pointer, self locking APIs */
>  virMacMap *macmap;
>  
> @@ -239,6 +244,30 @@ virNetworkObjSetFloorSum(virNetworkObj *obj,
>  }
>  
>  
> +virFirewall **
> +virNetworkObjGetFwRemovalPtr(virNetworkObj *obj)
> +{
> +return &obj->fwRemoval;
> +}
> +
> +
> +virFirewall *
> +virNetworkObjGetFwRemoval(virNetworkObj *obj)
> +{
> +return obj->fwRemoval;
> +}
> +
> +
> +void
> +virNetworkObjSetFwRemoval(virNetworkObj *obj,
> +  virFirewall *fwRemoval)
> +{
> +obj->fwRemoval = fwRemoval;
> +/* give it a name so it's identifiable in the XML */
> +virFirewallSetName(fwRemoval, "fwRemoval");
> +}

This call is segv'ing because 'fwRemoval' is NULL if you're
starting virnetworkd from an existing host which is currently
running old libvirt, as the FW rules won't be in the status
XML file. I guess this pristine upgrade scenario is not present
on your own machine.

Just putting an 'if (fwRemoval)' around the SetName call
seems like the right action.

Unless you want to immediately synthesize a virFirewall
object with historically accurate rollback rules when
loading status XML. Might make later code simpler if
you can rely on this being non-NULL ?


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v3 20/27] conf: add a virFirewall object to virNetworkObj

2024-04-25 Thread Laine Stump

On 4/25/24 12:33 PM, Daniel P. Berrangé wrote:

On Thu, Apr 25, 2024 at 01:38:26AM -0400, Laine Stump wrote:

This virFirewall object will store the list of actions required to
remove the firewall that was added for the currently active instance
of the network, so it has been named "fwRemoval" (and when parsed into
XML, the  element will have the name "fwRemoval").

There are no uses of the fwRemoval object in the virNetworkObj yet,
but everything is in place to add it to the XML when formatted, parse
it from the XML when reading network status, and free the virFirewall
object when the virNetworkObj is freed.

Signed-off-by: Laine Stump 
Reviewed-by: Daniel P. Berrangé 
---
Change from V2:
* add name='fwRemoval' to the network status  so it
   can be positively identified in the XML

  src/conf/virnetworkobj.c | 46 
  src/conf/virnetworkobj.h | 11 ++
  src/libvirt_private.syms |  3 +++
  3 files changed, 60 insertions(+)

diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index d5aa121e20..fef4c69004 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -55,6 +55,11 @@ struct _virNetworkObj {
  
  unsigned int taint;
  
+/* fwRemoval contains all commands needed to remove the firewall

+ * that was added for this network.
+ */
+virFirewall *fwRemoval;
+
  /* Immutable pointer, self locking APIs */
  virMacMap *macmap;
  
@@ -239,6 +244,30 @@ virNetworkObjSetFloorSum(virNetworkObj *obj,

  }
  
  
+virFirewall **

+virNetworkObjGetFwRemovalPtr(virNetworkObj *obj)
+{
+return &obj->fwRemoval;
+}
+
+
+virFirewall *
+virNetworkObjGetFwRemoval(virNetworkObj *obj)
+{
+return obj->fwRemoval;
+}
+
+
+void
+virNetworkObjSetFwRemoval(virNetworkObj *obj,
+  virFirewall *fwRemoval)
+{
+obj->fwRemoval = fwRemoval;
+/* give it a name so it's identifiable in the XML */
+virFirewallSetName(fwRemoval, "fwRemoval");
+}


This call is segv'ing because 'fwRemoval' is NULL if you're
starting virnetworkd from an existing host which is currently
running old libvirt, as the FW rules won't be in the status
XML file. I guess this pristine upgrade scenario is not present
on your own machine.

Just putting an 'if (fwRemoval)' around the SetName call
seems like the right action.


Yeah, I found that this morning and fixed it as well (exactly as you 
suggest). The danger of staying up late to finish something is that your 
brain *thinks* you retested everything after that last fix, but often 
you didn't :-/




Unless you want to immediately synthesize a virFirewall
object with historically accurate rollback rules when
loading status XML. Might make later code simpler if
you can rely on this being non-NULL ?


Well, having that pointer NULL is significant, as that's how I tell the 
difference between "there was no fwRemoval in the status"  (i.e. "use 
the old hardcoded method of removing an iptables firewall") and "there 
was an fwRemoval, but it was empty (i.e. "do nothing", which I'm 
assuming will be the case when I add in an update of egarver's support 
for native firewalld backend).


Although I suppose that empty  would still have backend='firewalld'>, so maybe I don't need to differentiate.

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v3 12/27] network: support setting firewallBackend from network.conf

2024-04-25 Thread Laine Stump

On 4/25/24 12:30 PM, Daniel P. Berrangé wrote:

On Thu, Apr 25, 2024 at 01:38:18AM -0400, Laine Stump wrote:

It still can have only one useful value ("iptables"), but once a 2nd
value is supported, it will be selectable by setting
"firewall_backend=nftables" in /etc/libvirt/network.conf.

If firewall_backend isn't set in network.conf, then libvirt will check
to see if the iptables binary is present on the system and set
firewallBackend to iptables - if no iptables binary is found, that is
considered a fatal error (since no networks can be started anyway), so
an error is logged and startup of the network driver fails.

NB: network.conf is itself created from network.conf.in at build time,
and the advertised default setting of firewall_backend (in a commented
out line) is set from the meson_options.txt setting
"firewall_backend". This way the conf file will have correct
information no matter what default backend is chosen at build time.

Signed-off-by: Laine Stump 
Reviewed-by: Daniel P. Berrangé 
---
Changes from V2:

  * make failure to find iptables during network driver init fatal

  * recognize firewall_backend setting in meson_options.txt and
propagate it through to:

 * meson-config.h (along with a numeric constant
   FIREWALL_BACKEND_DEFAULT_IPTABLES, which can be used for
   conditional compilation)
 * network.conf - default setting and comments use @FIREWALL_BACKEND@
 * test_libvirtd_network.aug.in so that default firewall backend
   for testing is set properly (this required calisthenics similar to
   qemu_user_group_hack_conf in the qemu driver's meson.build
   (see commit 64a7b8203bf)

  meson.build  |  5 +++
  meson_options.txt|  1 +
  src/network/bridge_driver.c  | 22 ++-
  src/network/bridge_driver_conf.c | 50 
  src/network/bridge_driver_conf.h |  3 ++
  src/network/bridge_driver_linux.c|  6 ++-
  src/network/bridge_driver_nop.c  |  6 ++-
  src/network/bridge_driver_platform.h |  6 ++-
  src/network/libvirtd_network.aug |  5 ++-
  src/network/meson.build  | 22 ++-
  src/network/network.conf.in  |  8 
  src/network/test_libvirtd_network.aug.in |  3 ++
  tests/networkxml2firewalltest.c  |  2 +-
  13 files changed, 119 insertions(+), 20 deletions(-)




diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c
index a2edafa837..25cbbf8933 100644
--- a/src/network/bridge_driver_conf.c
+++ b/src/network/bridge_driver_conf.c
@@ -25,6 +25,7 @@
  #include "datatypes.h"
  #include "virlog.h"
  #include "virerror.h"
+#include "virfile.h"
  #include "virutil.h"
  #include "bridge_driver_conf.h"
  
@@ -62,6 +63,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,

 const char *filename)
  {
  g_autoptr(virConf) conf = NULL;
+g_autofree char *firewallBackendStr = NULL;
  
  /* if file doesn't exist or is unreadable, ignore the "error" */

  if (access(filename, R_OK) == -1)


Not visible, but here we are about to do 'return 0'.

This means that if the config file doesn't exist at all, then
none of this method below will run.

The logic for detecting the "best" firewall backend in the
"} else {" clause below will thus never run. So without a
config file on disk, it will always implicitly get the
iptables backend set.



oops. I hadn't noticed that since I always have a config file.


We should set all defaults upfront, which means detectnig
nft vs iptables. Then set an override later when reading
the config, if it exists.


Okay, how about this:

1) check for the existence of both backends and save that in a couple of 
bools.


2) based on the the results of (1) plus meson_options.txt (or meson 
commandline) setting of firewall_backend, do a preliminary setting of 
backend (or fail if neither is found).


3) If the config file has a firewall_backend setting, look at the 
results of (1) to make sure it's available, and set that (if it's *not* 
available, do we fail? Or ignore? I'm inclined to say fail)





@@ -73,6 +75,54 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
G_GNUC_UNUSED,
  
  /* use virConfGetValue*(conf, ...) functions to read any settings into cfg */
  
+if (virConfGetValueString(conf, "firewall_backend", &firewallBackendStr) < 0)

+return -1;
+
+if (firewallBackendStr) {
+int backend = virFirewallBackendTypeFromString(firewallBackendStr);
+
+if (backend < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unknown value for 'firewall_backend' in network.conf: 
'%1$s'"),
+   firewallBackendStr);
+return -1;
+}
+
+cfg->firewallBackend = backend;
+VIR_INFO("using firewall_backend setting from network.conf: '%s'",
+ virFirewallB

Re: [PATCH v3 00/27] [PATCH v3 00/27] native support for nftables in virtual network driver

2024-04-25 Thread Daniel P . Berrangé
On Thu, Apr 25, 2024 at 01:38:06AM -0400, Laine Stump wrote:
> V2: 
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/5RTZ6PC3N3CO6X353QUHLVOL43SWQ4JD/
> 
> This patch series enables libvirt to use nftables rules rather than
> iptables *when setting up virtual networks* (it does *not* add
> nftables support to the nwfilter driver).

I deployed on my machine and restarted virtnetworkd, with nft backend
active. I have 2 networks running, and got the following result

table ip libvirt {
chain INPUT {
type filter hook input priority filter; policy accept;
counter packets 363 bytes 30801 jump LIBVIRT_INP
}

chain FORWARD {
type filter hook forward priority filter; policy accept;
counter packets 1 bytes 76 jump LIBVIRT_FWX
counter packets 1 bytes 76 jump LIBVIRT_FWI
counter packets 1 bytes 76 jump LIBVIRT_FWO
}

chain OUTPUT {
type filter hook output priority filter; policy accept;
counter packets 286 bytes 107221 jump LIBVIRT_OUT
}

chain LIBVIRT_INP {
iifname "virbr0" udp dport 53 counter packets 0 bytes 0 accept
iifname "virbr0" tcp dport 53 counter packets 0 bytes 0 accept
iifname "virbr0" udp dport 67 counter packets 1 bytes 320 accept
iifname "virbr0" tcp dport 67 counter packets 0 bytes 0 accept
iifname "virbr1" udp dport 53 counter packets 0 bytes 0 accept
iifname "virbr1" tcp dport 53 counter packets 0 bytes 0 accept
iifname "virbr1" udp dport 67 counter packets 0 bytes 0 accept
iifname "virbr1" tcp dport 67 counter packets 0 bytes 0 accept
}

chain LIBVIRT_OUT {
oifname "virbr0" udp dport 53 counter packets 0 bytes 0 accept
oifname "virbr0" tcp dport 53 counter packets 0 bytes 0 accept
oifname "virbr0" udp dport 68 counter packets 1 bytes 336 accept
oifname "virbr0" tcp dport 68 counter packets 0 bytes 0 accept
oifname "virbr1" udp dport 53 counter packets 0 bytes 0 accept
oifname "virbr1" tcp dport 53 counter packets 0 bytes 0 accept
oifname "virbr1" udp dport 68 counter packets 0 bytes 0 accept
oifname "virbr1" tcp dport 68 counter packets 0 bytes 0 accept
}

chain LIBVIRT_FWO {
ip saddr 192.168.122.0/24 iifname "virbr0" counter packets 1 
bytes 76 accept
iifname "virbr0" counter packets 0 bytes 0 reject
ip saddr 192.168.100.0/24 iifname "virbr1" counter packets 0 
bytes 0 accept
iifname "virbr1" counter packets 0 bytes 0 reject
}

chain LIBVIRT_FWI {
oifname "virbr0" ip daddr 192.168.122.0/24 ct state 
established,related counter packets 0 bytes 0 accept
oifname "virbr0" counter packets 0 bytes 0 reject
oifname "virbr1" ip daddr 192.168.100.0/24 ct state 
established,related counter packets 0 bytes 0 accept
oifname "virbr1" counter packets 0 bytes 0 reject
}

chain LIBVIRT_FWX {
iifname "virbr0" oifname "virbr0" counter packets 0 bytes 0 
accept
iifname "virbr1" oifname "virbr1" counter packets 0 bytes 0 
accept
}

chain POSTROUTING {
type nat hook postrouting priority srcnat; policy accept;
counter packets 2 bytes 136 jump LIBVIRT_PRT
}

chain LIBVIRT_PRT {
ip saddr 192.168.122.0/24 ip daddr 224.0.0.0/24 counter packets 
0 bytes 0 return
ip saddr 192.168.122.0/24 ip daddr 255.255.255.255 counter 
packets 0 bytes 0 return
meta l4proto tcp ip saddr 192.168.122.0/24 ip daddr != 
192.168.122.0/24 counter packets 0 bytes 0 masquerade to :1024-65535
meta l4proto udp ip saddr 192.168.122.0/24 ip daddr != 
192.168.122.0/24 counter packets 1 bytes 76 masquerade to :1024-65535
ip saddr 192.168.122.0/24 ip daddr != 192.168.122.0/24 counter 
packets 0 bytes 0 masquerade
ip saddr 192.168.100.0/24 ip daddr 224.0.0.0/24 counter packets 
0 bytes 0 return
ip saddr 192.168.100.0/24 ip daddr 255.255.255.255 counter 
packets 0 bytes 0 return
meta l4proto tcp ip saddr 192.168.100.0/24 ip daddr != 
192.168.100.0/24 counter packets 0 bytes 0 masquerade to :1024-65535
meta l4proto udp ip saddr 192.168.100.0/24 ip daddr != 
192.168.100.0/24 counter packets 0 bytes 0 masquerade to :1024-65535
ip saddr 192.168.100.0/24 ip daddr != 192.168.100.0/24 counter 
packets 0 bytes 0 masquerade
}
}


I also got some 'ip6' rules, but I'll omit that, since my points will
be identical for both.

I compared these rules, against the nft r

Re: [PATCH v3 12/27] network: support setting firewallBackend from network.conf

2024-04-25 Thread Daniel P . Berrangé
On Thu, Apr 25, 2024 at 01:04:10PM -0400, Laine Stump wrote:
> On 4/25/24 12:30 PM, Daniel P. Berrangé wrote:
> > On Thu, Apr 25, 2024 at 01:38:18AM -0400, Laine Stump wrote:
> > > It still can have only one useful value ("iptables"), but once a 2nd
> > > value is supported, it will be selectable by setting
> > > "firewall_backend=nftables" in /etc/libvirt/network.conf.
> > > 
> > > If firewall_backend isn't set in network.conf, then libvirt will check
> > > to see if the iptables binary is present on the system and set
> > > firewallBackend to iptables - if no iptables binary is found, that is
> > > considered a fatal error (since no networks can be started anyway), so
> > > an error is logged and startup of the network driver fails.
> > > 
> > > NB: network.conf is itself created from network.conf.in at build time,
> > > and the advertised default setting of firewall_backend (in a commented
> > > out line) is set from the meson_options.txt setting
> > > "firewall_backend". This way the conf file will have correct
> > > information no matter what default backend is chosen at build time.
> > > 
> > > Signed-off-by: Laine Stump 
> > > Reviewed-by: Daniel P. Berrangé 
> > > ---
> > > Changes from V2:
> > > 
> > >   * make failure to find iptables during network driver init fatal
> > > 
> > >   * recognize firewall_backend setting in meson_options.txt and
> > > propagate it through to:
> > > 
> > >  * meson-config.h (along with a numeric constant
> > >FIREWALL_BACKEND_DEFAULT_IPTABLES, which can be used for
> > >conditional compilation)
> > >  * network.conf - default setting and comments use @FIREWALL_BACKEND@
> > >  * test_libvirtd_network.aug.in so that default firewall backend
> > >for testing is set properly (this required calisthenics similar to
> > >qemu_user_group_hack_conf in the qemu driver's meson.build
> > >(see commit 64a7b8203bf)
> > > 
> > >   meson.build  |  5 +++
> > >   meson_options.txt|  1 +
> > >   src/network/bridge_driver.c  | 22 ++-
> > >   src/network/bridge_driver_conf.c | 50 
> > >   src/network/bridge_driver_conf.h |  3 ++
> > >   src/network/bridge_driver_linux.c|  6 ++-
> > >   src/network/bridge_driver_nop.c  |  6 ++-
> > >   src/network/bridge_driver_platform.h |  6 ++-
> > >   src/network/libvirtd_network.aug |  5 ++-
> > >   src/network/meson.build  | 22 ++-
> > >   src/network/network.conf.in  |  8 
> > >   src/network/test_libvirtd_network.aug.in |  3 ++
> > >   tests/networkxml2firewalltest.c  |  2 +-
> > >   13 files changed, 119 insertions(+), 20 deletions(-)
> > 
> > 
> > > diff --git a/src/network/bridge_driver_conf.c 
> > > b/src/network/bridge_driver_conf.c
> > > index a2edafa837..25cbbf8933 100644
> > > --- a/src/network/bridge_driver_conf.c
> > > +++ b/src/network/bridge_driver_conf.c
> > > @@ -25,6 +25,7 @@
> > >   #include "datatypes.h"
> > >   #include "virlog.h"
> > >   #include "virerror.h"
> > > +#include "virfile.h"
> > >   #include "virutil.h"
> > >   #include "bridge_driver_conf.h"
> > > @@ -62,6 +63,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
> > > G_GNUC_UNUSED,
> > >  const char *filename)
> > >   {
> > >   g_autoptr(virConf) conf = NULL;
> > > +g_autofree char *firewallBackendStr = NULL;
> > >   /* if file doesn't exist or is unreadable, ignore the "error" */
> > >   if (access(filename, R_OK) == -1)
> > 
> > Not visible, but here we are about to do 'return 0'.
> > 
> > This means that if the config file doesn't exist at all, then
> > none of this method below will run.
> > 
> > The logic for detecting the "best" firewall backend in the
> > "} else {" clause below will thus never run. So without a
> > config file on disk, it will always implicitly get the
> > iptables backend set.
> > 
> 
> oops. I hadn't noticed that since I always have a config file.
> 
> > We should set all defaults upfront, which means detectnig
> > nft vs iptables. Then set an override later when reading
> > the config, if it exists.
> 
> Okay, how about this:
> 
> 1) check for the existence of both backends and save that in a couple of
> bools.
> 
> 2) based on the the results of (1) plus meson_options.txt (or meson
> commandline) setting of firewall_backend, do a preliminary setting of
> backend (or fail if neither is found).

Yes, fail if neither is present.

> 
> 3) If the config file has a firewall_backend setting, look at the results of
> (1) to make sure it's available, and set that (if it's *not* available, do
> we fail? Or ignore? I'm inclined to say fail)

Yes, fail if user's request choice is not available.

> > > +} else {
> > > +
> > > +/* no .conf setting, so see what this host supports by looking
> > > + * for binaries used by the backends, and se

Re: Revisiting parallel save/restore

2024-04-25 Thread Jim Fehlig via Devel

On 4/17/24 5:12 PM, Jim Fehlig wrote:

Hi All,

While Fabiano has been working on improving save/restore performance in qemu, 
I've been tinkering with the same in libvirt. The end goal is to introduce a new 
VIR_DOMAIN_SAVE_PARALLEL flag for save/restore, along with a 
VIR_DOMAIN_SAVE_PARAM_PARALLEL_CONNECTIONS parameter to specify the number of 
concurrent channels used for the save/restore. Recall Claudio previously posted 
a patch series implementing parallel save/restore completely in libvirt, using 
qemu's multifd functionality [1].


A good starting point on this journey is supporting the new mapped-ram 
capability in qemu 9.0 [2]. Since mapped-ram is a new on-disk format, I assume 
we'll need a new QEMU_SAVE_VERSION 3 when using it? Otherwise I'm not sure how 
to detect if a saved image is in mapped-ram format vs the existing, sequential 
stream format.


While hacking on a POC, I discovered the save data cookie and assume the use of 
mapped-ram could be recorded there?


IIUC, mapped-ram cannot be used with the exiting 'fd:' migration URI and instead 
must use 'file:'. Does qemu advertise support for that? I couldn't find it. If 
not, 'file:' (available in qemu 8.2) predates mapped-ram, so in theory we could 
live without the advertisement.


It's also not clear when we want to enable the mapped-ram capability. Should it 
always be enabled if supported by the underlying qemu? One motivation for 
creating the mapped-ram was to support direct-io of the migration stream in 
qemu, in which case it could be tied to VIR_DOMAIN_SAVE_BYPASS_CACHE. E.g. the 
mapped-ram capability is enabled when user specifies 
VIR_DOMAIN_SAVE_BYPASS_CACHE && user-provided path results in a seekable fd && 
qemu supports mapped-ram?


Comments/suggestions on these topics are much appreciated :-).

Looking ahead, should the mapped-ram capability be required for supporting the 
VIR_DOMAIN_SAVE_PARALLEL flag?


I think the answer is yes, otherwise we'd need something in libvirt like 
Claudio's original series to manage multifd channels writing to fixed offsets in 
the save file.


Regards,
Jim

As I understand, parallel save/restore was 
another motivation for creating the mapped-ram feature. It allows multifd 
threads to write exclusively to the offsets provided by mapped-ram. Can multiple 
multifd threads concurrently write to an fd without mapped-ram?


Regards,
Jim

[1] 
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/3Y5GMS6A4QS4IXWDKFFV3A2FO5YMCFES/
[2] 
https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/mapped-ram.rst?ref_type=heads

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v1 15/20] node_device_udev: Pass the driver state as parameter in preparation for the next commit

2024-04-25 Thread Jim Fehlig via Devel

On 4/23/24 3:41 AM, Marc Hartmayer wrote:

On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé 
 wrote:

On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote:

On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé 
 wrote:

On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote:

On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma  
wrote:

On 4/19/24 9:49 AM, Marc Hartmayer wrote:

It's better practice for all functions called by the threads to

pass the driver


[…snip…]



Hmm, is the downside of doing a full codebase reformat greater than
always doing the code formatting manually? Probably it is, otherwise it
would have already be done :)

If we would have such a big commit we could list it in a
`.git-blame-ignore-revs` file so it will ignored by git blames.


Yes, that's great for git blame. The bigger problem though is that
a bulk reformat will immediately kill the ability of distro
maintainers to cleanly cherry-pick patches across the big reformat
commit. Cherry-picking the reformat likely won't be clean either,
so they'll be faced with many patches needing manual editting.


Yes, but isn't that already the case most of the time? But since I do
not backport libvirt fixes, I cannot answer this for sure :)


As a downstream package maintainer, I always shake my fist at these types of 
reformat changes when they break clean cherry-picks. But I also understand 
progress, modernization, etc can't be hamstrung by downstream convenience :-).


Regards,
Jim



Anyhow, we shouldn't misuse this mail thread for the side discussion :)
Is it worth starting a separate thread for it or is the answer a clear
NACK?



The tricky question is whether it is none the less worthwhile doing
it. The distro maintainer pain will be very real, but also somewhat
timelimited, as the need to backport fixes to a given release
as it ages.


If the people doing the backporting are more or less libvirt developers,
it might be a good trade for them in the long run.



With regards,
Daniel
--
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|


___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 1/6] migration: Remove 'skipped' field from MigrationStats

2024-04-25 Thread Markus Armbruster
Fabiano Rosas  writes:

> The 'skipped' field of the MigrationStats struct has been deprecated
> in 8.1. Time to remove it.
>
> Deprecation commit 7b24d32634 ("migration: skipped field is really
> obsolete.").
>
> Signed-off-by: Fabiano Rosas 
> ---
>  docs/about/deprecated.rst   | 6 --
>  docs/about/removed-features.rst | 6 ++
>  migration/migration-hmp-cmds.c  | 2 --
>  migration/migration.c   | 2 --
>  qapi/migration.json | 8 
>  5 files changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 7b548519b5..4d9d6bf2da 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -488,12 +488,6 @@ option).
>  Migration
>  -
>  
> -``skipped`` MigrationStats field (since 8.1)
> -
> -
> -``skipped`` field in Migration stats has been deprecated.  It hasn't
> -been used for more than 10 years.
> -
>  ``inc`` migrate command option (since 8.2)
>  ''
>  
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index f9cf874f7b..9873f59bee 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -614,6 +614,12 @@ was superseded by ``sections``.
>  Member ``section-size`` in the return value of ``query-sgx-capabilities``
>  was superseded by ``sections``.
>  
> +``query-migrate`` return value member ``skipped`` (removed in 9.1)
> +''
> +
> +Member ``skipped`` of the ``MigrationStats`` struct hasn't been used
> +for more than 10 years. Removed with no replacement.
> +
>  Human Monitor Protocol (HMP) commands
>  -
>  
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 7e96ae6ffd..28f776d06d 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -105,8 +105,6 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
> info->ram->total >> 10);
>  monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
> info->ram->duplicate);
> -monitor_printf(mon, "skipped: %" PRIu64 " pages\n",
> -   info->ram->skipped);
>  monitor_printf(mon, "normal: %" PRIu64 " pages\n",
> info->ram->normal);
>  monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
> diff --git a/migration/migration.c b/migration/migration.c
> index 696762bc64..3b433fdb31 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1149,8 +1149,6 @@ static void populate_ram_info(MigrationInfo *info, 
> MigrationState *s)
>  info->ram->transferred = migration_transferred_bytes();
>  info->ram->total = ram_bytes_total();
>  info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
> -/* legacy value.  It is not used anymore */
> -info->ram->skipped = 0;
>  info->ram->normal = stat64_get(&mig_stats.normal_pages);
>  info->ram->normal_bytes = info->ram->normal * page_size;
>  info->ram->mbps = s->mbps;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8c65b90328..401b8e24ac 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -23,9 +23,6 @@
>  #
>  # @duplicate: number of duplicate (zero) pages (since 1.2)
>  #
> -# @skipped: number of skipped zero pages.  Always zero, only provided
> -# for compatibility (since 1.5)
> -#
>  # @normal: number of normal pages (since 1.2)
>  #
>  # @normal-bytes: number of normal bytes sent (since 1.2)
> @@ -63,16 +60,11 @@
>  # between 0 and @dirty-sync-count * @multifd-channels.  (since
>  # 7.1)
>  #
> -# Features:
> -#
> -# @deprecated: Member @skipped is always zero since 1.5.3
> -#
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationStats',
>'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> 'duplicate': 'int',
> -   'skipped': { 'type': 'int', 'features': [ 'deprecated' ] },
> 'normal': 'int',
> 'normal-bytes': 'int', 'dirty-pages-rate': 'int',
> 'mbps': 'number', 'dirty-sync-count': 'int',

Reviewed-by: Markus Armbruster 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 0/6] migration removals & deprecations

2024-04-25 Thread Markus Armbruster
Doesn't apply for me.  What's your base?
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org