Re: [PATCH] virnetdevbandwidth.c: Put a limit to "quantum"
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
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
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
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
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
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
'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
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
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
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
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
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
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
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
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
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
*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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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