Re: [PATCH v1 15/20] node_device_udev: Pass the driver state as parameter in preparation for the next commit
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
>> via parameter and not global variables. Easier to test and cleaner.
>>
[…snip…]
>>
>>
>> static int
>> -udevProcessPCI(struct udev_device *device,
>> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device
>> *device,
>> virNodeDeviceDef *def)
>
> While there are exceptions, the general coding style is to have a single
> argument per line for function definitions.
Okay. BTW, why is there no .clangformat configuration available for
Libvirt? :/
[…snip…]
>> return 0;
>> @@ -1995,15 +1995,15 @@ udevSetupSystemDev(void)
>> static void
>> nodeStateInitializeEnumerate(void *opaque)
>> {
>> -struct udev *udev = opaque;
>> udevEventData *priv = driver->privateData;
>> +struct udev *udev = opaque;
>
> unnecessary change?
Will remove.
>
>>
>> /* Populate with known devices */
>> -if (udevEnumerateDevices(udev) != 0)
>> +if (udevEnumerateDevices(driver, udev) != 0)
>> goto error;
>> /* Load persistent mdevs (which might not be activated yet) and
>> additional
>>* information about active mediated devices from mdevctl */
>> -if (nodeDeviceUpdateMediatedDevices() != 0)
>> +if (nodeDeviceUpdateMediatedDevices(driver) != 0)
>> goto error;
>>
>>cleanup:
>> @@ -2051,9 +2051,11 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
>>
>>
>> static void
>> -mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED)
>> +mdevctlUpdateThreadFunc(void *opaque)
>> {
>> -if (nodeDeviceUpdateMediatedDevices() < 0)
>> +virNodeDeviceDriverState *driver_state = opaque;
>> +
>> +if (nodeDeviceUpdateMediatedDevices(driver_state) < 0)
>> VIR_WARN("mdevctl failed to update mediated devices");
>> }
>>
>> @@ -2070,7 +2072,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED,
>> void *opaque)
>> }
>>
>> if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc,
>> -"mdevctl-thread", false, NULL) < 0) {
>> +"mdevctl-thread", false, driver) < 0) {
>> virReportSystemError(errno, "%s",
>>_("failed to create mdevctl thread"));
>> }
>
>
>
> Reviewed-by: Jonathon Jongsma
Thanks.
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
___
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 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 > >> via parameter and not global variables. Easier to test and cleaner. > >> > > […snip…] > > >> > >> > >> static int > >> -udevProcessPCI(struct udev_device *device, > >> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device > >> *device, > >> virNodeDeviceDef *def) > > > > While there are exceptions, the general coding style is to have a single > > argument per line for function definitions. > > Okay. BTW, why is there no .clangformat configuration available for > Libvirt? :/ There is no combination of clangformat settings that can match libvirt code style. If we were startnig again from scratch we would of course want to match a defined clangformat style, but unless we're willing to bulk reformat the codebase we are stuck with what we have. 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 v1 17/20] node_device_udev: Call `nodeDeviceUpdateMediatedDevices` directly
On Mon, Apr 22, 2024 at 05:43 PM +0200, Boris Fiuczynski wrote: > This could be quashed with patch 3 but I am also fine with this if you > do not want to spend the effort. > > Reviewed-by: Boris Fiuczynski Will squash it. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 ___ 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 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 >> >> via parameter and not global variables. Easier to test and cleaner. >> >> >> >> […snip…] >> >> >> >> >> >> >> static int >> >> -udevProcessPCI(struct udev_device *device, >> >> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct >> >> udev_device *device, >> >> virNodeDeviceDef *def) >> > >> > While there are exceptions, the general coding style is to have a single >> > argument per line for function definitions. >> >> Okay. BTW, why is there no .clangformat configuration available for >> Libvirt? :/ > > There is no combination of clangformat settings that can match > libvirt code style. If we were startnig again from scratch we > would of course want to match a defined clangformat style, > but unless we're willing to bulk reformat the codebase we are > stuck with what we have. 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. > > > 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 :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 ___ 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 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 > >> >> via parameter and not global variables. Easier to test and cleaner. > >> >> > >> > >> […snip…] > >> > >> >> > >> >> > >> >> static int > >> >> -udevProcessPCI(struct udev_device *device, > >> >> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct > >> >> udev_device *device, > >> >> virNodeDeviceDef *def) > >> > > >> > While there are exceptions, the general coding style is to have a single > >> > argument per line for function definitions. > >> > >> Okay. BTW, why is there no .clangformat configuration available for > >> Libvirt? :/ > > > > There is no combination of clangformat settings that can match > > libvirt code style. If we were startnig again from scratch we > > would of course want to match a defined clangformat style, > > but unless we're willing to bulk reformat the codebase we are > > stuck with what we have. > > 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. 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. 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 v1 15/20] node_device_udev: Pass the driver state as parameter in preparation for the next commit
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 :) 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 :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 01/27] util/network: move viriptables.[ch] from util to network directory
On Sun, Apr 21, 2024 at 10:53:09PM -0400, Laine Stump wrote:
> These functions are only ever used by the network driver, and are so
> specific to the network driver's usage of iptables that they likely
> won't ever be used elsewhere. The files are renamed to
> network_iptables.[ch] to be more in line with driver-specific file
> naming conventions.
>
> Signed-off-by: Laine Stump
> ---
> po/POTFILES | 2 +-
> src/libvirt_private.syms | 31 ---
> src/network/bridge_driver_linux.c | 2 +-
> src/network/meson.build | 1 +
> .../network_iptables.c} | 7 +++--
> .../network_iptables.h} | 2 +-
> src/util/meson.build | 1 -
> 7 files changed, 8 insertions(+), 38 deletions(-)
> rename src/{util/viriptables.c => network/network_iptables.c} (99%)
> rename src/{util/viriptables.h => network/network_iptables.h} (99%)
Reviewed-by: Daniel P. Berrangé
with one question below...
> diff --git a/src/util/viriptables.c b/src/network/network_iptables.c
> similarity index 99%
> rename from src/util/viriptables.c
> rename to src/network/network_iptables.c
> index 018021bc1b..bf6e3065f5 100644
> --- a/src/util/viriptables.c
> +++ b/src/network/network_iptables.c
> @@ -1,5 +1,5 @@
> /*
> - * viriptables.c: helper APIs for managing iptables
> + * network_iptables.c: helper APIs for managing iptables in network driver
> *
> * Copyright (C) 2007-2014 Red Hat, Inc.
> *
> @@ -27,13 +27,14 @@
> #include
>
> #include "internal.h"
> -#include "viriptables.h"
> #include "virfirewalld.h"
> #include "virerror.h"
> #include "virlog.h"
> #include "virhash.h"
> +#include "virenum.h"
Presume this virenum.h addition is an accident, that should be in a
later commit ?
> +#include "network_iptables.h"
>
> -VIR_LOG_INIT("util.iptables");
> +VIR_LOG_INIT("network.iptables");
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
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 v2 03/27] network: make all iptables functions used only in network_iptables.c static
On Sun, Apr 21, 2024 at 10:53:11PM -0400, Laine Stump wrote: > Now that the toplevel iptables functions have been moved out of the > linux bridge driver into network_iptables.c, all of the utility > functions are used only within that same file, so simplify it. > > Signed-off-by: Laine Stump > --- > src/network/network_iptables.c | 52 ++--- > src/network/network_iptables.h | 130 - > 2 files changed, 26 insertions(+), 156 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 02/27] network: move all functions manipulating iptables rules into network_iptables.c
On Sun, Apr 21, 2024 at 10:53:10PM -0400, Laine Stump wrote: > Although initially we will add exactly the same rules for the nftables > backend, the two may (hopefully) soon diverge as we take advantage of > nftables features that weren't available in iptables. When we do that, > there will need to be a different version of these functions (currently in > bridge_driver_linux.c) for each backend: > > networkAddFirewallRules() > networkRemoveFirewallRules() > networkSetupPrivateChains() > > Although it will mean duplicating some amount of code (with just the > function names changed) for the nftables backend, this patch moves all > of the rule-related code in the above three functions into iptables*() > functions in network_iptables.c, and changes the functions in > bridge_driver_linux.c to call the iptables*() functions. When we make > a different backend, it will only need to make equivalents of those 3 > functions publicly available to the upper layer. > > Signed-off-by: Laine Stump > --- > src/network/bridge_driver_linux.c | 556 + > src/network/network_iptables.c| 562 +- > src/network/network_iptables.h| 7 +- > 3 files changed, 574 insertions(+), 551 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 04/27] util: #define the names used for private packet filter chains
On Sun, Apr 21, 2024 at 10:53:12PM -0400, Laine Stump wrote: > Signed-off-by: Laine Stump > --- > src/network/network_iptables.c | 51 +++--- > 1 file changed, 29 insertions(+), 22 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 06/27] util: rename virNetFilterAction to iptablesAction, and add VIR_ENUM_DECL/IMPL
On Sun, Apr 21, 2024 at 10:53:14PM -0400, Laine Stump wrote:
> I had originally named these as VIR_NETFILTER_* because I assumed the
> same enum would eventually be used by our nftables backend as well as
> iptables. But it turns out that in most cases it's not possible to
> delete an nftables rule, so we just never used the enum anyway, so
> this patch is renaming the values to IPTABLES_ACTION_*, and taking
> advantage of the newly defined (via VIR_ENUM_DECL/IMPL)
> iptablesActionTypeToString() to replace all the ternary operators used
> to translate the enum into a string for the iptables commandline with
> iptablesActionTypeToString().
>
> Signed-off-by: Laine Stump
> ---
> src/network/network_iptables.c | 125 ++---
> 1 file changed, 68 insertions(+), 57 deletions(-)
Reviewed-by: Daniel P. Berrangé
>
> diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c
> index 31af9e0db6..d7e749adf0 100644
> --- a/src/network/network_iptables.c
> +++ b/src/network/network_iptables.c
> @@ -46,10 +46,21 @@ VIR_LOG_INIT("network.iptables");
> #define VIR_IPTABLES_FWD_X_CHAIN "LIBVIRT_FWX"
> #define VIR_IPTABLES_NAT_POSTROUTE_CHAIN "LIBVIRT_PRT"
This is where the extra '#include "virenum.h"' from patch 1 ought to
have instead arrived I presume.
>
> -enum {
> -VIR_NETFILTER_INSERT = 0,
> -VIR_NETFILTER_DELETE
> -};
> +typedef enum {
> +IPTABLES_ACTION_INSERT,
> +IPTABLES_ACTION_APPEND,
> +IPTABLES_ACTION_DELETE,
> +
> +IPTABLES_ACTION_LAST
> +} iptablesAction;
> +
> +VIR_ENUM_DECL(iptablesAction);
> +VIR_ENUM_IMPL(iptablesAction,
> + IPTABLES_ACTION_LAST,
> + "--insert",
> + "--append",
> + "--delete",
> +);
>
> typedef struct {
> const char *parent;
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 v2 07/27] util: check for 0 args when applying iptables rule
On Sun, Apr 21, 2024 at 10:53:15PM -0400, Laine Stump wrote: > In normal practice a virFirewallCmd should never have 0 args by the > time it gets to the Apply stage, but at some time while debugging one > of the other patches in this series, exactly that happened (due to a > bug that was since squashed), and having a check for it helped > debugging, so let's permanently check for it. > > Signed-off-by: Laine Stump > --- > src/util/virfirewall.c | 6 ++ > 1 file changed, 6 insertions(+) Reviewed-by: Daniel P. Berrangé 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 v2 09/27] util: determine ignoreErrors value when creating virFirewallCmd, not when applying
On Sun, Apr 21, 2024 at 10:53:17PM -0400, Laine Stump wrote: > We know at the time a virFirewallCmd is created (with > virFirewallAddCmd*()) whether or not we will later want to ignore > errors encountered when attempting to apply that command - if > ignoreErrors is set in the AddCmd or if the group has already had > VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS set, then we ignore the errors. > > Rather than setting the fwCmd->ignoreErrors only according to the arg > sent to virFirewallAddCmdFull(), and then later (at ApplyCmd-time) > combining that with the group transactionFlags setting (and passing it > all the way down the call chain), just combine the two flags right > away and store this final value in fwCmd->ignoreErrors when the > virFirewallCmd is created (thus avoiding the need to look at anything > other than fwCmd->ignoreErrors at the time the command is applied). Once > that is done, we can simply grab ignoreErrors from the object down in > virFirewallApply() rather than cluttering up the argument list on the > entire call chain. > > Signed-off-by: Laine Stump > --- > src/util/virfirewall.c | 28 +--- > 1 file changed, 13 insertions(+), 15 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 10/27] util/network: new virFirewallBackend enum
On Sun, Apr 21, 2024 at 10:53:18PM -0400, Laine Stump wrote:
> (This paragraph is for historical reference only, described only to
> avoid confusion of past use of the name with its new use) In a past
> life, virFirewallBackend had been a private static in virfirewall.c
> that was set at daemon init time, and used to globally (i.e. for all
> drivers in the daemon) determine whether to directly execute iptables
> commands, or to run them indirectly via the firewalld passthrough
> API. This was removed in commit d566cc55, since we decided that using
> the firewalld passthrough API is never appropriate.
>
> Now the same enum, virFirewallBackend, is being reintroduced, with a
> different meaning and usage pattern. It will be used to pick between
> using nftables commands or iptables commands (in either case directly
> handled by libvirt, *not* via firewalld). Additionally, rather than
> being a static known only within virfirewall.c and applying to all
> firewall commands for all drivers, each virFirewall object will have
> its own backend setting, which will be set during virFirewallNew() by
> the driver who wants to add a firewall rule.
>
> This will allow the nwfilter and network drivers to each have their
> own backend setting, even when they coexist in a single unified
> daemon. At least as important as that, it will also allow an instance
> of the network driver to remove iptables rules that had been added by
> a previous instance, and then add nftables rules for the new instance
> (in the case that an admin, or possibly an update, switches the driver
> backend from iptables to nftable)
>
> Initially, the enum will only have one usable value -
> VIR_FIREWALL_BACKEND_IPTABLES, and that will be hardcoded into all
> calls to virFirewallNew(). The other enum value (along with a method
> of setting it for each driver) will be added later, when it can be
> used (when the nftables backend is in the code).
>
> Signed-off-by: Laine Stump
> ---
> src/libvirt_private.syms | 3 +++
> src/network/network_iptables.c| 6 +++---
> src/nwfilter/nwfilter_ebiptables_driver.c | 16
> src/util/virebtables.c| 4 ++--
> src/util/virfirewall.c| 16 +++-
> src/util/virfirewall.h| 12 +++-
> tests/virfirewalltest.c | 20 ++--
> 7 files changed, 52 insertions(+), 25 deletions(-)
>
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index 56d43bfdde..489482fd83 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -35,6 +35,11 @@
>
> VIR_LOG_INIT("util.firewall");
>
> +VIR_ENUM_IMPL(virFirewallBackend,
> + VIR_FIREWALL_BACKEND_LAST,
> + "UNSET", /* not yet set */
> + "iptables");
> +
snip
> diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
> index 956bf0e2bf..7f0fee047d 100644
> --- a/src/util/virfirewall.h
> +++ b/src/util/virfirewall.h
> @@ -34,9 +35,18 @@ typedef enum {
> VIR_FIREWALL_LAYER_LAST,
> } virFirewallLayer;
>
> -virFirewall *virFirewallNew(void);
> +typedef enum {
> +VIR_FIREWALL_BACKEND_UNSET,
> +VIR_FIREWALL_BACKEND_IPTABLES,
> +
> +VIR_FIREWALL_BACKEND_LAST,
> +} virFirewallBackend;
We're forcing the callers to all pass VIR_FIREWALL_BACKEND_IPTABLES,
so I'm wondering if we actually need to have VIR_FIREWALL_BACKEND_UNSET
at all ? If we eliminate it, then that removes need to check for this
illegal value and report errors I guess.
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 v2 11/27] network: add (empty) network.conf file to distribution files
On Sun, Apr 21, 2024 at 10:53:19PM -0400, Laine Stump wrote: > Signed-off-by: Laine Stump > --- > libvirt.spec.in | 3 ++ > src/network/libvirtd_network.aug | 36 > src/network/meson.build | 11 > src/network/network.conf | 3 ++ > src/network/test_libvirtd_network.aug.in | 2 ++ > 5 files changed, 55 insertions(+) > create mode 100644 src/network/libvirtd_network.aug > create mode 100644 src/network/network.conf > create mode 100644 src/network/test_libvirtd_network.aug.in Reviewed-by: Daniel P. Berrangé 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 v2 12/27] network: support setting firewallBackend from network.conf
On Sun, Apr 21, 2024 at 10:53:20PM -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 not, it will be left as "unset", which
> (once multiple backends are available) will trigger an appropriate
> error message the first time we attempt to add a rule.
>
> Signed-off-by: Laine Stump
> ---
> src/network/bridge_driver.c | 22 +++--
> src/network/bridge_driver_conf.c | 40
> 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/network.conf | 8 +
> src/network/test_libvirtd_network.aug.in | 3 ++
> tests/networkxml2firewalltest.c | 2 +-
> 10 files changed, 83 insertions(+), 18 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index d89700c6ee..38e4ab84ad 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> diff --git a/src/network/bridge_driver_conf.c
> b/src/network/bridge_driver_conf.c
> index a2edafa837..9769ee06b5 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)
> @@ -73,6 +75,44 @@ 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));
Since the BACKEND enum has "unset" as a valid entry, and you're checking
'backend < 0' here, this allows the user to explicitly do
firewall_backend="UNSET"
To me this is another reason to eliminate the "UNSET" backend enum value.
> +
> +} 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;
> +
> +/* virFindFileInPath() uses g_find_program_in_path(),
> + * which allows absolute paths, and verifies that
> + * the file is executable.
> +*/
> +if ((iptablesInPath = virFindFileInPath(IPTABLES)))
> +cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES;
> +
> +if (cfg->firewallBackend == VIR_FIREWALL_BACKEND_UNSET)
Rather than checking against "UNSET", this could just be an 'else'
clause, and a fatal error.
Using a fatal error would mean the virnetworkd will fail to start,
since and journalctl will give the explanation.
If we only report it later at time of first usage, then the app
user will see it, but they're not in a position to fix it. Showing
a failed service looks to give more attention to the admin.
Of course they should not get into this situation in the first
place with a packaged distro, since the distro should have added
deps to pull in either iptables or nftables or both.
> +VIR_INFO("firewall_backend not set, and no usable backend
> auto-detected");
> +else
> +VIR_INFO("using auto-detected firewall_backend: '%s'",
> + virFirewallBackendTypeToString(cfg->firewallBackend));
> +}
> +
> return 0;
> }
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: ht
Re: [PATCH v2 13/27] network: framework to call backend-specific function to init private filter chains
On Sun, Apr 21, 2024 at 10:53:21PM -0400, Laine Stump wrote:
> Modify networkSetupPrivateChains() in the network driver to accept a
> firewallBackend argument so it will know which backend to call. (right
> now it always calls the iptables version of the lower level function,
> but in the future it could instead call the nftables version based on
> configuration).
>
> But networkSetupPrivateChains() was being called with virOnce(), and
> virOnce() doesn't support calling functions that require an argument
> (it's based on pthread_once(), which accepts no arguments, so it's not
> something we can easily fix in our implementation of virOnce()). To
> solve this dilemma, this patch eliminates use of virOnce() by adding a
> static lock, and putting all of networkSetupPrivateChains() (including
> the setting of "chainInitDone") inside a lock guard - now the places
> that used to call it via virOnce() can safely call it directly
> instead (adding in the necessary argument to specify backend).
>
> (If it turns out to be significant, we could optimize this by checking
> for chainInitDone outside the lock guard, returning immediately if
> it's already set, and then moving the setting of chainInitDone up to
> the top of the guarded section.)
>
> Signed-off-by: Laine Stump
> ---
> src/network/bridge_driver_linux.c | 65 ++-
> 1 file changed, 47 insertions(+), 18 deletions(-)
>
> diff --git a/src/network/bridge_driver_linux.c
> b/src/network/bridge_driver_linux.c
> index c2ef27f251..20671e3ec5 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -34,25 +34,53 @@ VIR_LOG_INIT("network.bridge_driver_linux");
>
> #define PROC_NET_ROUTE "/proc/net/route"
>
> -static virOnceControl createdOnce;
> +static virMutex chainInitLock = VIR_MUTEX_INITIALIZER;
> static bool chainInitDone; /* true iff networkSetupPrivateChains was ever
> called */
>
> static virErrorPtr errInitV4;
> static virErrorPtr errInitV6;
>
> -/* Usually only called via virOnce, but can also be called directly in
> - * response to firewalld reload (if chainInitDone == true)
> - */
> -static void networkSetupPrivateChains(void)
> +static void
> +networkFirewallBackendUnsetError(void)
> {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("firewall_backend wasn't set, and no usable setting
> could be auto-detected"));
> +}
> +
> +
> +static int
> +networkFirewallSetupPrivateChains(virFirewallBackend backend,
> + virFirewallLayer layer)
> +{
> +switch (backend) {
> +case VIR_FIREWALL_BACKEND_IPTABLES:
> +return iptablesSetupPrivateChains(layer);
> +
> +case VIR_FIREWALL_BACKEND_UNSET:
> +case VIR_FIREWALL_BACKEND_LAST:
> +networkFirewallBackendUnsetError();
for _LAST and default: cases, you should use
virReportEnumRangeError(virFirewallBackend, backend)
> +return -1;
> +}
> +return 0;
> +}
> +
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 v2 22/27] meson: stop looking for iptables/ip6tables/ebtables at build time
On Sun, Apr 21, 2024 at 10:53:30PM -0400, Laine Stump wrote: > This was the only reason we required the iptables and ebtables > packages at build time, and many other external commands already have > their binaries found at runtime by looking through $PATH (virCommand > automatically does this), so we may as well do it for these commands > as well. > > Inspired-by: 6aa2fa38b04b802f137e51ebbeb4ca9b67487575 > Signed-off-by: Laine Stump > --- > meson.build | 3 --- > src/network/bridge_driver_conf.c | 2 ++ > src/util/virfirewall.h | 5 + > 3 files changed, 7 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 23/27] rpm: drop BuildRequires for iptables and ebtables
On Sun, Apr 21, 2024 at 10:53:31PM -0400, Laine Stump wrote: > The only reason for requiring these was so that meson could search for > the binary location, and the previous patch eliminated that, so we no > longer need them at build time. > > Signed-off-by: Laine Stump > --- > libvirt.spec.in | 2 -- > 1 file changed, 2 deletions(-) Reviewed-by: Daniel P. Berrangé though I'd suggest it could just be squashed into the previous patch > diff --git a/libvirt.spec.in b/libvirt.spec.in > index bde25c6f6e..05f7a7e7c0 100644 > --- a/libvirt.spec.in > +++ b/libvirt.spec.in > @@ -356,8 +356,6 @@ BuildRequires: sanlock-devel >= 2.4 > BuildRequires: libpcap-devel >= 1.5.0 > BuildRequires: libnl3-devel > BuildRequires: libselinux-devel > -BuildRequires: iptables > -BuildRequires: ebtables > # For modprobe > BuildRequires: kmod > BuildRequires: cyrus-sasl-devel > -- > 2.44.0 > ___ > Devel mailing list -- devel@lists.libvirt.org > To unsubscribe send an email to devel-le...@lists.libvirt.org 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 v2 26/27] network: prefer the nftables backend over iptables
On Sun, Apr 21, 2024 at 10:53:34PM -0400, Laine Stump wrote:
> The initial patches to support nftables for virtual networks left
> iptables as the default backend.
>
> The only functional difference between the two backends is that the
> nftables backend doesn't add any rules to fix up the checksum of DHCP
> packets, which will cause failures on guests with very old OSes
> (e.g. RHEL5) that have a virtio-net network interface using vhost
> packet processing (the default), connected to a libvirt virtual
> network, and configured to acquire the interface IP using DHCP. Since
> RHEL5 has been out of support for several years already, we might as
> well start off nftables support right by making it the default.
>
> In the extremely unlikely case that this causes a problem for anyone,
> they can work around the failure by adding " to
> the guest element.
>
> Signed-off-by: Laine Stump
> ---
> src/network/bridge_driver_conf.c | 6 +++---
> src/network/network.conf | 9 ++---
> src/network/test_libvirtd_network.aug.in | 2 +-
> 3 files changed, 10 insertions(+), 7 deletions(-)
I wonder if we shouldn't make the default firewall backend be
a meson_options.txt parameter.
If a distro rebases libvirt in their existing release, they
probably don't want the firewall backend silently changing
as a side effect. A meson option would let them turn on the
new behaviour for only new releases. We could make the meson
option default to 'nft' though.
> diff --git a/src/network/bridge_driver_conf.c
> b/src/network/bridge_driver_conf.c
> index f1159ed245..0139ece5ad 100644
> --- a/src/network/bridge_driver_conf.c
> +++ b/src/network/bridge_driver_conf.c
> @@ -106,10 +106,10 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg
> G_GNUC_UNUSED,
> * which allows absolute paths, and verifies that
> * the file is executable.
> */
> -if ((iptablesInPath = virFindFileInPath(IPTABLES)))
> -cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES;
> -else if ((nftInPath = virFindFileInPath(NFT)))
> +if ((nftInPath = virFindFileInPath(NFT)))
> cfg->firewallBackend = VIR_FIREWALL_BACKEND_NFTABLES;
> +else if ((iptablesInPath = virFindFileInPath(IPTABLES)))
> +cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES;
>
> if (cfg->firewallBackend == VIR_FIREWALL_BACKEND_UNSET)
> VIR_INFO("firewall_backend not set, and no usable backend
> auto-detected");
> diff --git a/src/network/network.conf b/src/network/network.conf
> index 630c4387a1..31723bccd5 100644
> --- a/src/network/network.conf
> +++ b/src/network/network.conf
> @@ -12,8 +12,11 @@
> # iptables - use iptables commands to construct the firewall
> # nftables - use nft commands to construct the firewall
> #
> -# For backward compatibility, and to reduce surprises, the
> -# default setting is "iptables".
> +# If firewall_backend isn't explicitly specified here, libvirt
> +# will default to using nftables if the "nft" command is available
> +# on the host, otherwise it will use iptables if the "iptables"
> +# command is available. If neither is available, then libvirt
> +# will log an error the first time any network is started.
> #
> # (NB: switching from one backend to another while there are active
> # virtual networks *is* supported. The change will take place the
> @@ -21,4 +24,4 @@
> # virtual networks will have their old firewalls removed, and then
> # reloaded using the new backend.)
> #
> -#firewall_backend = "iptables"
> +#firewall_backend = "nftables"
> diff --git a/src/network/test_libvirtd_network.aug.in
> b/src/network/test_libvirtd_network.aug.in
> index 3aa7b4cc22..81a6256919 100644
> --- a/src/network/test_libvirtd_network.aug.in
> +++ b/src/network/test_libvirtd_network.aug.in
> @@ -2,4 +2,4 @@ module Test_libvirtd_network =
>@CONFIG@
>
>test Libvirtd_network.lns get conf =
> -{ "firewall_backend" = "iptables" }
> +{ "firewall_backend" = "nftables" }
> --
> 2.44.0
> ___
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-le...@lists.libvirt.org
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 v2 27/27] RFC: spec: change iptables/ebtables from Requires to Recommends, add nftables
On Sun, Apr 21, 2024 at 10:53:35PM -0400, Laine Stump wrote:
> We really shouldn't be requiring ebtables and iptables any more, since
> they don't always need to be used. Likewise, we probably should at
> least Recommend nftables, even though it's pretty much always
> installed already anyway.
>
> (Changing Requires to Recommends for the nwfilter package is a bit
> premature, since it currently will always require iptables and
> ebtables to function properly, but changing those to Recommends leads
> to a much smaller list of dependent packages removed by "dnf rm
> iptables/ebtables").
>
> Signed-off-by: Laine Stump
> ---
> libvirt.spec.in | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 05f7a7e7c0..66b328671d 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -592,7 +592,8 @@ Summary: Network driver plugin for the libvirtd daemon
> Requires: libvirt-daemon-common = %{version}-%{release}
> Requires: libvirt-libs = %{version}-%{release}
> Requires: dnsmasq >= 2.41
> -Requires: iptables
> +Recommends: iptables
> +Recommends: nftables
Or we use a bool expression:
Requires: (iptables or nftables)
which guarantees at least one is present and thus no possibility of
a broken install
>
> %description daemon-driver-network
> The network driver plugin for the libvirtd daemon, providing
> @@ -603,8 +604,8 @@ bridge capabilities.
> Summary: Nwfilter driver plugin for the libvirtd daemon
> Requires: libvirt-daemon-common = %{version}-%{release}
> Requires: libvirt-libs = %{version}-%{release}
> -Requires: iptables
> -Requires: ebtables
> +Recommends: iptables
> +Recommends: ebtables
This looks premature since we've not provided an nft backend option
for nwfilter. Thus the only effect of this change is to guarantee
the ability to create a broken instalation
When the time comes though we would do
Requires: (iptables or nftables)
Requires: (ebtables if iptables)
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 v2 05/27] util: change name of virFirewallRule to virFirewallCmd
On Sun, Apr 21, 2024 at 10:53:13PM -0400, Laine Stump wrote: > These objects aren't rules, they are commands that are executed that > may create a firewall rule, delete a firewall rule, or simply list the > existing firewall rules. It's confusing for the objects to be called > "Rule" (especially in the case of the function > virFirewallRemoveRule(), which doesn't remove a rule from the > firewall, it takes one of the objects out of the list of commands to > execute! In order to remove a rule from the host's firewall, you have > to Add a "rule" (now "cmd" aka command) to the list that will, when > applied/run, remove a rule from the host firewall.) > > Changing the name to virFirewallCmd makes it all much less confusing. > > Signed-off-by: Laine Stump > --- > src/libvirt_private.syms | 16 +- > src/network/network_iptables.c| 286 +++ > src/nwfilter/nwfilter_ebiptables_driver.c | 988 +++--- > src/util/virebtables.c| 32 +- > src/util/virfirewall.c| 223 +++-- > src/util/virfirewall.h| 54 +- > tests/virfirewalltest.c | 404 - > 7 files changed, 1000 insertions(+), 1003 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 08/27] util: add -w/--concurrent when applying a FirewallCmd rather than when building it
On Sun, Apr 21, 2024 at 10:53:16PM -0400, Laine Stump wrote: > We will already need a separate function for virFirewallApplyCmd for > iptables vs. nftables, but the only reason for needing a separate > function for virFirewallAddCmd* is that iptables/ebtables need to have > an extra arg added for locking (to prevent multiple iptables commands > from running at the same time). We can just as well add in the > -w/--concurrent during virFirewallApplyCmd, so move the arg-add to > ApplyCmd to keep AddCmd simple. > > Signed-off-by: Laine Stump > --- > src/util/virfirewall.c | 27 +-- > 1 file changed, 13 insertions(+), 14 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 15/27] util: implement rollback rule autocreation for iptables commands
On Sun, Apr 21, 2024 at 10:53:23PM -0400, Laine Stump wrote:
> If the VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK flag is set, each time
> an iptables command is executed that is adding a rule or chain, a
> corresponding command that will *delete* the same rule/chain is
> constructed and added to the list of rollback commands. If we later
> want to undo the entire firewall, we can just run those commands.
>
> This isn't yet used anywhere, since
> VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK isn't being set.
>
> Signed-off-by: Laine Stump
> ---
> src/util/virfirewall.c | 55 --
> 1 file changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index 274c5179ed..8cc551d6e2 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -471,7 +471,7 @@ void virFirewallStartTransaction(virFirewall *firewall,
> * Returns the virFirewallTransactionFlags for the currently active
> * group (transaction) in @firewall.
> */
> -static virFirewallTransactionFlags G_GNUC_UNUSED
> +static virFirewallTransactionFlags
> virFirewallTransactionGetFlags(virFirewall *firewall)
> {
> return firewall->groups[firewall->currentGroup]->actionFlags;
> @@ -526,16 +526,25 @@ virFirewallCmdToString(const char *cmd,
> }
>
>
> +#define VIR_IPTABLES_ARG_IS_INSERT(arg) \
> +(STREQ(arg, "--insert") || STREQ(arg, "-I") || \
> + STREQ(arg, "--append") || STREQ(arg, "-A"))
IS_CREATE feels like slightly less misleadnig name,
given this matches "append" too, and 'create' pairs
with 'delete' nicely.
> static int
> -virFirewallApplyCmdDirect(virFirewallCmd *fwCmd,
> - char **output)
> +virFirewallCmdIptablesApply(virFirewall *firewall,
> +virFirewallCmd *fwCmd,
> +char **output)
> {
> -size_t i;
> const char *bin = virFirewallLayerCommandTypeToString(fwCmd->layer);
> +bool checkRollback = (virFirewallTransactionGetFlags(firewall) &
> + VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK);
> +bool needRollback = false;
> g_autoptr(virCommand) cmd = NULL;
> g_autofree char *cmdStr = NULL;
> -int status;
> g_autofree char *error = NULL;
> +size_t i;
> +int status;
>
> if (!bin) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -559,8 +568,13 @@ virFirewallApplyCmdDirect(virFirewallCmd *fwCmd,
> break;
> }
>
> -for (i = 0; i < fwCmd->argsLen; i++)
> +for (i = 0; i < fwCmd->argsLen; i++) {
> +/* the -I/-A arg could be at any position in the list */
> +if (checkRollback && VIR_IPTABLES_ARG_IS_INSERT(fwCmd->args[i]))
> +needRollback = true;
> +
> virCommandAddArg(cmd, fwCmd->args[i]);
> +}
>
> cmdStr = virCommandToString(cmd, false);
> VIR_INFO("Running firewall command '%s'", NULLSTR(cmdStr));
> @@ -572,8 +586,10 @@ virFirewallApplyCmdDirect(virFirewallCmd *fwCmd,
> return -1;
>
> if (status != 0) {
> +/* the command failed, decide whether or not to report it */
> if (fwCmd->ignoreErrors) {
> VIR_DEBUG("Ignoring error running command");
> +return 0;
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to run firewall command %1$s: %2$s"),
> @@ -583,6 +599,31 @@ virFirewallApplyCmdDirect(virFirewallCmd *fwCmd,
> }
> }
>
> +/* the command was successful, see if we need to add a
> + * rollback command
> + */
> +
> +if (needRollback) {
> +virFirewallCmd *rollback
> += virFirewallAddRollbackCmd(firewall, fwCmd->layer, NULL);
> +g_autofree char *rollbackStr = NULL;
> +
> +for (i = 0; i < fwCmd->argsLen; i++) {
> +/* iptables --delete wants the entire commandline that
> + * was used for --insert but with s/insert/delete/
> + */
> +if (VIR_IPTABLES_ARG_IS_INSERT(fwCmd->args[i])) {
> +virFirewallCmdAddArg(firewall, rollback, "--delete");
> +} else {
> +virFirewallCmdAddArg(firewall, rollback, fwCmd->args[i]);
> +}
> +}
> +
> +rollbackStr =
> virFirewallCmdToString(virFirewallLayerCommandTypeToString(fwCmd->layer),
> + rollback);
> +VIR_DEBUG("Recording Rollback command '%s'", NULLSTR(rollbackStr));
> +}
> +
> return 0;
> }
>
> @@ -600,7 +641,7 @@ virFirewallApplyCmd(virFirewall *firewall,
> return -1;
> }
>
> -if (virFirewallApplyCmdDirect(fwCmd, &output) < 0)
> +if (virFirewallCmdIptablesApply(firewall, fwCmd, &output) < 0)
> return -1;
>
> if (fwCmd->queryCB && output) {
> --
> 2.44.0
> ___
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an
Re: [PATCH v2 16/27] network: turn on auto-rollback for the rules added for virtual networks
On Sun, Apr 21, 2024 at 10:53:24PM -0400, Laine Stump wrote:
> So far this will only affect what happens if there is some failure
> while applying the firewall rules; the rollback rules aren't yet
> persistent beyond that time. More work is needed to remember the
> rollback rules while the network is active, and use those rules to
> remove the firewall for the network when it is destroyed.
>
> Signed-off-by: Laine Stump
> ---
> src/network/network_iptables.c | 15 +++
> tests/networkxml2firewalltest.c | 9 -
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c
> index db35a4c5a0..467d43c1e9 100644
> --- a/src/network/network_iptables.c
> +++ b/src/network/network_iptables.c
> @@ -1599,7 +1599,7 @@ iptablesAddFirewallRules(virNetworkDef *def)
> virNetworkIPDef *ipdef;
> g_autoptr(virFirewall) fw =
> virFirewallNew(VIR_FIREWALL_BACKEND_IPTABLES);
>
> -virFirewallStartTransaction(fw, 0);
> +virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK);
>
> iptablesAddGeneralFirewallRules(fw, def);
>
> @@ -1610,17 +1610,8 @@ iptablesAddFirewallRules(virNetworkDef *def)
> return -1;
> }
>
> -virFirewallStartRollback(fw, 0);
> -
> -for (i = 0;
> - (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i));
> - i++) {
> -if (iptablesRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0)
> -return -1;
> -}
> -iptablesRemoveGeneralFirewallRules(fw, def);
> -
> -virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
> +virFirewallStartTransaction(fw, (VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS |
> +
> VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK));
> iptablesAddChecksumFirewallRules(fw, def);
>
> return virFirewallApply(fw);
To this point
Reviewed-by: Daniel P. Berrangé
> diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
> index 3a9f409e2a..e61787daec 100644
> --- a/tests/networkxml2firewalltest.c
> +++ b/tests/networkxml2firewalltest.c
> @@ -79,7 +79,14 @@ testCommandDryRun(const char *const*args G_GNUC_UNUSED,
>void *opaque G_GNUC_UNUSED)
> {
> *status = 0;
> -*output = g_strdup("");
> +/* if arg[1] is -ae then this is an nft command,
> + * and the caller requested to get the handle
> + * of the newly added object in stdout
> + */
> +if (STREQ_NULLABLE(args[1], "-ae"))
> +*output = g_strdup("# handle 5309");
> +else
> +*output = g_strdup("");
Belongs in the later nft tests patch
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 v2 18/27] util: new functions virFirewallParseXML() and virFirewallFormat()
On Sun, Apr 21, 2024 at 10:53:26PM -0400, Laine Stump wrote:
> These functions convert a virFirewall object to/from XML so that it
> can be serialized to disk (in a virNetworkObj's status file) and
> restored later (e.g. after libvirtd/virtnetworkd is restarted).
>
> Signed-off-by: Laine Stump
> ---
> src/libvirt_private.syms | 2 +
> src/util/virfirewall.c | 217 +++
> src/util/virfirewall.h | 9 ++
> 3 files changed, 228 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e3dcb353b7..aa253a238b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2413,10 +2413,12 @@ virFirewallCmdAddArgList;
> virFirewallCmdAddArgSet;
> virFirewallCmdGetArgCount;
> virFirewallCmdToString;
> +virFirewallFormat;
> virFirewallFree;
> virFirewallGetBackend;
> virFirewallNew;
> virFirewallNewFromRollback;
> +virFirewallParseXML;
> virFirewallRemoveCmd;
> virFirewallStartRollback;
> virFirewallStartTransaction;
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index 57d45abc17..684569c760 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -40,6 +40,14 @@ VIR_ENUM_IMPL(virFirewallBackend,
>"UNSET", /* not yet set */
>"iptables");
>
> +VIR_ENUM_DECL(virFirewallLayer);
> +VIR_ENUM_IMPL(virFirewallLayer,
> + VIR_FIREWALL_LAYER_LAST,
> + "ethernet",
> + "ipv4",
> + "ipv6",
> +);
> +
> typedef struct _virFirewallGroup virFirewallGroup;
>
> VIR_ENUM_DECL(virFirewallLayerCommand);
> @@ -810,3 +818,212 @@ virFirewallNewFromRollback(virFirewall *original,
>
> return 0;
> }
> +
> +
> +/* virFirewallGetFlagsFromNode:
> + * @node: the xmlNode to check for an ignoreErrors attribute
> + *
> + * A short helper to get the setting of the ignorErrors attribute from
> + * an xmlNode. Returns -1 on error (with error reported), or the
> + * VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS bit set/reset according to
> + * the value of the attribute.
> + */
> +static int
> +virFirewallGetFlagsFromNode(xmlNodePtr node)
> +{
> +virTristateBool ignoreErrors;
> +
> +if (virXMLPropTristateBool(node, "ignoreErrors", VIR_XML_PROP_NONE,
> &ignoreErrors) < 0)
> +return -1;
> +
> +if (ignoreErrors == VIR_TRISTATE_BOOL_YES)
> +return VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS;
> +return 0;
> +}
> +
> +
> +/**
> + * virFirewallParseXML:
> + * @firewall: pointer to virFirewall* to fill in with new virFirewall object
> + *
> + * Construct a new virFirewall object according to the XML in
> + * xmlNodePtr. Return 0 (and new object) on success, or -1 (with
> + * error reported) on error.
> + *
> + * Example of element XML:
> + *
> + *
> + *
> + *
> + *
> + * arg1
> + * arg2
> + * ...
> + *
> + *
> + *
> + * ...
> +
> + * ...
> + *
> + * ...
> + *
> + */
> +int
> +virFirewallParseXML(virFirewall **firewall,
> +xmlNodePtr node,
> +xmlXPathContextPtr ctxt)
> +{
> +g_autoptr(virFirewall) newfw = NULL;
> +virFirewallBackend backend;
> +g_autofree xmlNodePtr *groupNodes = NULL;
> +ssize_t ngroups;
> +size_t g;
> +VIR_XPATH_NODE_AUTORESTORE(ctxt);
> +
> +ctxt->node = node;
> +
> +ngroups = virXPathNodeSet("./group", ctxt, &groupNodes);
> +if (ngroups < 0)
> +return -1;
> +if (ngroups == 0)
> +return 0;
> +
> +if (virXMLPropEnum(node, "backend", virFirewallBackendTypeFromString,
> + VIR_XML_PROP_REQUIRED, &backend) < 0) {
> +return -1;
> +}
> +
> +newfw = virFirewallNew(backend);
> +
> +for (g = 0; g < ngroups; g++) {
> +int flags = 0;
> +g_autofree xmlNodePtr *actionNodes = NULL;
> +ssize_t nactions;
> +size_t a;
> +
> +ctxt->node = groupNodes[g];
> +nactions = virXPathNodeSet("./action", ctxt, &actionNodes);
> +if (nactions < 0)
> +return -1;
> +if (nactions == 0)
> +continue;
> +
> +if ((flags = virFirewallGetFlagsFromNode(groupNodes[g])) < 0)
> +return -1;
> +
> +virFirewallStartTransaction(newfw, flags);
> +
> +for (a = 0; a < nactions; a++) {
> +g_autofree xmlNodePtr *argsNodes = NULL;
> +ssize_t nargs;
> +size_t i;
> +virFirewallLayer layer;
> +virFirewallCmd *action;
> +bool ignoreErrors;
> +
> +ctxt->node = actionNodes[a];
> +
> +if (!(ctxt->node = virXPathNode("./args", ctxt)))
> +continue;
> +
> +if ((flags = virFirewallGetFlagsFromNode(actionNodes[a])) < 0)
> +return -1;
> +
> +ignoreErrors = flags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS;
> +
> +if (virXMLPropEnum(actionNodes[a], "layer
Re: [PATCH v2 19/27] conf: add a virFirewall object to virNetworkObj
On Sun, Apr 21, 2024 at 10:53:27PM -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". > > 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 > --- > src/conf/virnetworkobj.c | 39 +++ > src/conf/virnetworkobj.h | 11 +++ > src/libvirt_private.syms | 3 +++ > 3 files changed, 53 insertions(+) Reviewed-by: Daniel P. Berrangé 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 v2 20/27] network: use previously saved list of firewall removal commands
On Sun, Apr 21, 2024 at 10:53:28PM -0400, Laine Stump wrote: > When destroying a network, the network driver has always assumed that > it knew what firewall rules had been added as the network was > started. This was usually correct - I only recall one time in the past > that the firewall rules added by libvirt were changed. But if the > exact rules used for a network *were* ever changed from one > build/version of libvirt to another, then we would end up attempting > to remove rules that hadn't been added, and could possibly *not* > remove rules that had been added. > > The solution to this to not make such brash assumptions about the > past, but instead to save (in the network status object at network > start time) a list of all the rules needed to remove the rules that > were added for the network, and then use that saved list during > network destroy to remove exactly what was previous added. > > Beyond making net-destroy more precise, there are other benefits: > > 1) We can change the details of the rules we add for networks from one > build/release of libvirt to another and painlessly upgrade. > > 2) The user can switch from one firewall backend to another by simply > changing the setting in network.conf and restarting > libvirtd/virtnetworkd. > > In both cases, the restarted libvirtd/virtnetworkd will remove all the > rules that had been previously added (based on the network status), > and then add new rules (saving the new removal commands back into the > network status) > > Signed-off-by: Laine Stump > > == > > NB: the current implementation saves only the commands necessary to > remove the network's firewall, and names this in the status > XML. It would be simple to instead save the *entire* virFirewall > object (thus also including the commands that were used to add the > firewall, as well as the commands needed to remove it) - although very > verbose, it's possible it could be useful when debugging a firewall > issue (since it's not obvious which rules were added for which network > when just looking at the output of "nft list ruleset". Alternately, we > could continue to store only the removal commands, but maybe change > the name of the element in XML from to (which > would leave the door open to expanding what is saved in the > future). Any opinions on this? IMHO we should just stick with recording the info we need from a functional POV. Reviewed-by: Daniel P. Berrangé 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 v2 24/27] network: add an nftables backend for network driver's firewall construction
On Sun, Apr 21, 2024 at 10:53:32PM -0400, Laine Stump wrote:
> Support using nftables to setup the firewall for each virtual network,
> rather than iptables. The initial implementation of the nftables
> backend creates (almost) exactly the same ruleset as the iptables
> backend, determined by running the following commands on a host that
> has an active virtual network:
>
> iptables-save >iptables.txt
> iptables-restore-translate -f iptables.txt
>
> (and the similar ip6tables-save/ip6tables-restore-translate for an
> IPv6 network). Correctness of the new backend was checked by comparing
> the output of:
>
> nft list ruleset
>
> when the backend is set to iptables and when it is set to nftables.
>
> This page was used as a guide:
>
>
> https://wiki.nftables.org/wiki-nftables/index.php/Moving_from_iptables_to_nftables
>
> The only differences between the rules created by the nftables backed
> vs. the iptables backend (aside from a few inconsequential changes in
> display order of some chains/options) are:
>
> 1) When we add nftables rules, rather than adding them in the
> system-created "filter" and "nat" tables, we add them in a private
> table (ie only we should be using it) created by us called "libvirt"
> (the system-created "filter" and "nat" tables can't be used because
> adding any rules to those tables directly with nft will cause failure
> of any legacy application attempting to use iptables when it tries to
> list the iptables rules (e.g. "iptables -S").
>
> (NB: in nftables only a single table is required for both nat and
> filter rules - the chains for each are differentiated by specifying
> different "hook" locations for the toplevel chain of each)
>
> 2) nftables doesn't support the "checksum mangle" rule (or any
> equivalent functionality) that we have historically added to our
> iptables rules, so the nftables rules we add have nothing related to
> checksum mangling.
>
> (NB: The result of (2) is that if you a) have a very old guest (RHEL5
> era or earlier) and b) that guest is using a virtio-net network
> device, and c) the virtio-net device is using vhost packet processing
> (the default) then DHCP on the guest will fail. You can work around
> this by adding to the XML for the
> guest).
>
> There are certainly much better nftables rulesets that could be used
> instead of those implemented here, and everything is in place to make
> future changes to the rules that are used simple and free of surprises
> (e.g. the rules that are added have coresponding "removal" commands
> added to the network status so that we will always remove exactly the
> rules that were previously added rather than trying to remove the
> rules that "this build of libvirt would have added" (which will be
> incorrect the first time we run a libvirt with a newly modified
> ruleset). For this initial implementation though, I wanted the
> nftables rules to be as identical to the iptables rules as possible,
> just to make it easier to verify that everything is working.
>
> The backend can be manually chosen using the firewall_backend setting
> in /etc/libvirt/network.conf. libvirtd/virtnetworkd will read this
> setting when it starts; if there is no explicit setting, it will look
> for iptables commands on the host and, if they are found, will select
> the iptables backend (this is the default for the sake of 100%
> backward compatibility), but if iptables commands aren't found, then
> it will use the nftables backend.
>
> (Since libvirt will automatically remove all its previous filter rules
> and re-add rules using the current backend setting for all active
> networks when it starts up, and the only noticeable change in behavior
> between the iptables and nftables backends is that noted in item (2)
> above, we could instead decide to make nftables the default backend
> rather than iptables - it all depends on how important it is to work
> properly on 15 year old guest OSes using DHCP with virtio-net
> interfaces).
>
> Signed-off-by: Laine Stump
> ---
> po/POTFILES | 1 +
> src/network/bridge_driver_conf.c | 3 +
> src/network/bridge_driver_linux.c | 18 +-
> src/network/meson.build | 1 +
> src/network/network.conf | 17 +-
> src/network/network_nftables.c| 940 ++
> src/network/network_nftables.h| 28 +
> src/util/virfirewall.c| 169 +-
> src/util/virfirewall.h| 2 +
> 9 files changed, 1174 insertions(+), 5 deletions(-)
> create mode 100644 src/network/network_nftables.c
> create mode 100644 src/network/network_nftables.h
>
> +if (needRollback) {
> +virFirewallCmd *rollback = virFirewallAddRollbackCmd(firewall,
> fwCmd->layer, NULL);
> +const char *handleStart = NULL;
> +size_t handleLen = 0;
> +g_autofree char *handleStr = NULL;
> +g_autofree char *rollbackStr = NULL;
> +
> +/* Search for "# handle n" in stdout of the nft a
Re: [PATCH v4 3/3] conf: nodedev: Fill active_config at XML parse time
On 4/19/24 8:38 AM, Boris Fiuczynski wrote: > On 4/17/24 17:17, Cole Robinson wrote: >> Commit v10.0.0-265-ge67bca23e4 added a `active_config` and >> `defined_config` to nodedev mdev internal XML handling. >> `defined_config` can be filled at XML parse time, but `active_config` >> must be filled in by nodedev driver. This wasn't implemented for the >> test driver however, which caused virt-manager test suite regressions. > > I still think that the mocking of state changes should be handled inside > of the test driver itself of the virNodeDeviceDriver in the > implementation the interfaces: > nodeDeviceCreateXML => creates a transient mdev from the XML (no > persistent config) > nodeDeviceDestroy => removes the active mdev (a transient mdev is > completely removed) > nodeDeviceDefineXML => creates a persistent mdev config from the XML > nodeDeviceUndefine => removes the persistent mdev config (if mdev is > active the active config remains) > nodeDeviceCreate => creates the active config from the persistent > config > > Therefore for mocking > * copy defined_config to active_config > * reset defined_config > * reset active_config > should be sufficient. > > Since there are only nodeDeviceCreateXML and nodeDeviceDestroy > implemented in the test driver the first two should do the trick. > OK, patches incoming which take this change out of the common parser. I did not fix the test driver API impls because they are unrelated to my goal of fixing the virt-manager test suite Thanks, Cole ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v5 1/3] test: make parsed nodedevs active and persistent
This was the implied default before nodedevs gained a notion of being inactive and transient. It also matches the implied default when parsing other object types Signed-off-by: Cole Robinson --- src/test/test_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..153ab7cdc2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,8 @@ testParseNodedevs(testDriver *privconn, return -1; } +virNodeDeviceObjSetPersistent(obj, true); +virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); virNodeDeviceObjEndAPI(&obj); } -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v5 2/3] test: Sync GetXML INACTIVE behavior with live driver
- Error if INACTIVE requested for transient object
- Force dumping INACTIVE XML when object is inactive
Signed-off-by: Cole Robinson
---
src/test/test_driver.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 153ab7cdc2..e7d2b6c866 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -7514,15 +7514,30 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
{
testDriver *driver = dev->conn->privateData;
virNodeDeviceObj *obj;
+virNodeDeviceDef *def;
char *ret = NULL;
virCheckFlags(VIR_NODE_DEVICE_XML_INACTIVE, NULL);
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
return NULL;
+def = virNodeDeviceObjGetDef(obj);
-ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);
+if (flags & VIR_NODE_DEVICE_XML_INACTIVE) {
+if (!virNodeDeviceObjIsPersistent(obj)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _("node device '%1$s' is not persistent"),
+ def->name);
+goto cleanup;
+}
+} else {
+if (!virNodeDeviceObjIsActive(obj))
+flags |= VIR_NODE_DEVICE_XML_INACTIVE;
+}
+ret = virNodeDeviceDefFormat(def, flags);
+
+ cleanup:
virNodeDeviceObjEndAPI(&obj);
return ret;
}
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v5 0/3] test: fix nodedev mdev XML regression
See last patch for explanation. First two patches are related bugfixes/improvements v5: - Changed impl to match Boris' suggestion Cole Robinson (3): test: make parsed nodedevs active and persistent test: Sync GetXML INACTIVE behavior with live driver test: nodedev: fill active_config at driver startup time src/conf/node_device_conf.c | 24 src/conf/node_device_conf.h | 3 +++ src/libvirt_private.syms| 1 + src/test/test_driver.c | 20 +++- tests/nodedevxml2xmltest.c | 18 +++--- 5 files changed, 50 insertions(+), 16 deletions(-) -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v5 3/3] test: nodedev: fill active_config at driver startup time
Commit v10.0.0-265-ge67bca23e4 added a `active_config` and
`defined_config` to nodedev mdev internal XML handling.
`defined_config` can be filled at XML parse time, but `active_config`
must be filled in by nodedev driver. This wasn't implemented for the
test driver however, which caused virt-manager test suite regressions.
Working example:
```
$ virsh --connect
test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml
nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110
css_0_0_0023
```
Broken example:
```
$ virsh --connect
test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml
nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110
css_0_0_0023
```
There's already code that does what we want in the test suite.
Move it to a shared function, and call it in test driver when
creating a nodedev from driver startup XML.
Signed-off-by: Cole Robinson
---
src/conf/node_device_conf.c | 24
src/conf/node_device_conf.h | 3 +++
src/libvirt_private.syms| 1 +
src/test/test_driver.c | 1 +
tests/nodedevxml2xmltest.c | 18 +++---
5 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5cfbd6a7eb..fe6d9a36b2 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2804,6 +2804,30 @@ virNodeDeviceCapsListExport(virNodeDeviceDef *def,
return ncaps;
}
+void
+virNodeDeviceSyncMdevActiveConfig(virNodeDeviceDef *def)
+{
+size_t i;
+virNodeDevCapsDef *caps;
+
+for (caps = def->caps; caps; caps = caps->next) {
+virNodeDevCapData *data = &caps->data;
+
+if (caps->data.type != VIR_NODE_DEV_CAP_MDEV)
+continue;
+
+data->mdev.active_config.type =
g_strdup(data->mdev.defined_config.type);
+for (i = 0; i < data->mdev.defined_config.nattributes; i++) {
+g_autoptr(virMediatedDeviceAttr) attr =
g_new0(virMediatedDeviceAttr, 1);
+
+attr->name =
g_strdup(data->mdev.defined_config.attributes[i]->name);
+attr->value =
g_strdup(data->mdev.defined_config.attributes[i]->value);
+VIR_APPEND_ELEMENT(data->mdev.active_config.attributes,
+ data->mdev.active_config.nattributes,
+ attr);
+}
+}
+}
#ifdef __linux__
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index f0a5333881..4b82636af7 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -470,3 +470,6 @@ virNodeDeviceUpdateCaps(virNodeDeviceDef *def);
int
virNodeDeviceCapsListExport(virNodeDeviceDef *def,
virNodeDevCapType **list);
+
+void
+virNodeDeviceSyncMdevActiveConfig(virNodeDeviceDef *def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 839fe4f545..3186dd6d23 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -888,6 +888,7 @@ virNodeDeviceGetPCIDynamicCaps;
virNodeDeviceGetSCSIHostCaps;
virNodeDeviceGetSCSITargetCaps;
virNodeDeviceGetWWNs;
+virNodeDeviceSyncMdevActiveConfig;
virNodeDeviceUpdateCaps;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index e7d2b6c866..d2d1bc43e3 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1272,6 +1272,7 @@ testParseNodedevs(testDriver *privconn,
virNodeDeviceObjSetPersistent(obj, true);
virNodeDeviceObjSetActive(obj, true);
virNodeDeviceObjSetSkipUpdateCaps(obj, true);
+virNodeDeviceSyncMdevActiveConfig(def);
virNodeDeviceObjEndAPI(&obj);
}
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index e918922672..814a817725 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -24,7 +24,6 @@ testCompareXMLToXMLFiles(const char *xml, const char
*outfile, unsigned int flag
int ret = -1;
virNodeDeviceDef *dev = NULL;
virNodeDevCapsDef *caps;
-size_t i;
if (virTestLoadFile(xml, &xmlData) < 0)
goto fail;
@@ -52,22 +51,11 @@ testCompareXMLToXMLFiles(const char *xml, const char
*outfile, unsigned int flag
data->storage.logical_block_size;
}
}
-
-if (caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
-!(flags & VIR_NODE_DEVICE_XML_INACTIVE)) {
-data->mdev.active_config.type =
g_strdup(data->mdev.defined_config.type);
-for (i = 0; i < data->mdev.defined_config.nattributes; i++) {
-g_autoptr(virMediatedDeviceAttr) attr =
g_new0(virMediatedDeviceAttr, 1);
-
-attr->name =
g_strdup(data->mdev.defined_config.attri
[PATCH v2 2/4] virfile: Introduce virFileReadValueBitmapAllowEmpty()
Some sysfs files contain either string representation of a bitmap
or just a newline character. An example of such file is:
/sys/devices/system/cpu/isolated. Our current implementation of
virFileReadValueBitmap() fails in the latter case, unfortunately.
Introduce a slightly modified version that accepts empty files.
Signed-off-by: Michal Privoznik
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 81 ++--
src/util/virfile.h | 2 +
3 files changed, 65 insertions(+), 19 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0ba6183010..2c7e4b45d3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2360,6 +2360,7 @@ virFileReadHeaderFD;
virFileReadHeaderQuiet;
virFileReadLimFD;
virFileReadValueBitmap;
+virFileReadValueBitmapAllowEmpty;
virFileReadValueInt;
virFileReadValueScaledInt;
virFileReadValueString;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index deaf4555fd..c769f7d650 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4365,26 +4365,12 @@ virFileReadValueScaledInt(unsigned long long *value,
const char *format, ...)
* used for small, interface-like files, so it should not be huge (subjective)
*/
#define VIR_FILE_READ_VALUE_STRING_MAX 4096
-/**
- * virFileReadValueBitmap:
- * @value: pointer to virBitmap * to be allocated and filled in with the value
- * @format, ...: file to read from
- *
- * Read int from @format and put it into @value.
- *
- * Return -2 for non-existing file, -1 on other errors and 0 if everything went
- * fine.
- */
-int
-virFileReadValueBitmap(virBitmap **value, const char *format, ...)
+static int
+virFileReadValueBitmapImpl(virBitmap **value,
+ const char *path,
+ bool allowEmpty)
{
g_autofree char *str = NULL;
-g_autofree char *path = NULL;
-va_list ap;
-
-va_start(ap, format);
-path = g_strdup_vprintf(format, ap);
-va_end(ap);
if (!virFileExists(path))
return -2;
@@ -4394,13 +4380,70 @@ virFileReadValueBitmap(virBitmap **value, const char
*format, ...)
virStringTrimOptionalNewline(str);
-*value = virBitmapParseUnlimited(str);
+if (allowEmpty) {
+*value = virBitmapParseUnlimitedAllowEmpty(str);
+} else {
+*value = virBitmapParseUnlimited(str);
+}
+
if (!*value)
return -1;
return 0;
}
+
+/**
+ * virFileReadValueBitmap:
+ * @value: pointer to virBitmap * to be allocated and filled in with the value
+ * @format, ...: file to read from
+ *
+ * Read int from @format and put it into @value.
+ *
+ * Returns: -2 for non-existing file,
+ * -1 on other errors (with error reported),
+ * 0 otherwise.
+ */
+int
+virFileReadValueBitmap(virBitmap **value, const char *format, ...)
+{
+g_autofree char *path = NULL;
+va_list ap;
+
+va_start(ap, format);
+path = g_strdup_vprintf(format, ap);
+va_end(ap);
+
+return virFileReadValueBitmapImpl(value, path, false);
+}
+
+
+/**
+ * virFileReadValueBitmapAllowEmpty:
+ * @value: pointer to virBitmap * to be allocated and filled in with the value
+ * @format, ...: file to read from
+ *
+ * Just like virFileReadValueBitmap(), except if the file is empty or contains
+ * nothing but spaces an empty bitmap is returned instead of an error.
+ *
+ * Returns: -2 for non-existing file,
+ * -1 on other errors (with error reported),
+ * 0 otherwise.
+ */
+int
+virFileReadValueBitmapAllowEmpty(virBitmap **value, const char *format, ...)
+{
+g_autofree char *path = NULL;
+va_list ap;
+
+va_start(ap, format);
+path = g_strdup_vprintf(format, ap);
+va_end(ap);
+
+return virFileReadValueBitmapImpl(value, path, true);
+}
+
+
/**
* virFileReadValueString:
* @value: pointer to char * to be allocated and filled in with the value
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 56fe309bce..7df3fcb840 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -354,6 +354,8 @@ int virFileReadValueUllongQuiet(unsigned long long *value,
const char *format, .
G_GNUC_PRINTF(2, 3);
int virFileReadValueBitmap(virBitmap **value, const char *format, ...)
G_GNUC_PRINTF(2, 3);
+int virFileReadValueBitmapAllowEmpty(virBitmap **value, const char *format,
...)
+ G_GNUC_PRINTF(2, 3);
int virFileReadValueScaledInt(unsigned long long *value, const char *format,
...)
G_GNUC_PRINTF(2, 3);
int virFileReadValueString(char **value, const char *format, ...)
--
2.43.2
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 1/4] virbitmap: Introduce virBitmapParseUnlimitedAllowEmpty()
Some sysfs files contain either string representation of a bitmap
or just a newline character. An example of such file is:
/sys/devices/system/cpu/isolated. Our current implementation of
virBitmapParseUnlimited() fails in the latter case,
unfortunately. Introduce a slightly modified version that accepts
empty files.
Signed-off-by: Michal Privoznik
---
src/libvirt_private.syms | 1 +
src/util/virbitmap.c | 40 +++-
src/util/virbitmap.h | 3 +++
tests/virbitmaptest.c| 40
4 files changed, 79 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 839fe4f545..0ba6183010 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1935,6 +1935,7 @@ virBitmapNextSetBit;
virBitmapOverlaps;
virBitmapParse;
virBitmapParseUnlimited;
+virBitmapParseUnlimitedAllowEmpty;
virBitmapSetAll;
virBitmapSetBit;
virBitmapSetBitExpand;
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index e48224d709..775bbf1532 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -368,6 +368,7 @@ virBitmapFormat(virBitmap *bitmap)
* @str: points to a string representing a human-readable bitmap
* @bitmap: a bitmap populated from @str
* @limited: Don't use self-expanding APIs, report error if bit exceeds bitmap
size
+ * @allowEmpty: Allow @str to be empty string or contain nothing but spaces
*
* This function is the counterpart of virBitmapFormat. This function creates
* a bitmap, in which bits are set according to the content of @str.
@@ -381,7 +382,8 @@ virBitmapFormat(virBitmap *bitmap)
static int
virBitmapParseInternal(const char *str,
virBitmap *bitmap,
- bool limited)
+ bool limited,
+ bool allowEmpty)
{
bool neg = false;
const char *cur = str;
@@ -389,13 +391,19 @@ virBitmapParseInternal(const char *str,
size_t i;
int start, last;
-if (!str)
+if (!str) {
+if (allowEmpty)
+return 0;
goto error;
+}
virSkipSpaces(&cur);
-if (*cur == '\0')
+if (*cur == '\0') {
+if (allowEmpty)
+return 0;
goto error;
+}
while (*cur != 0) {
/*
@@ -505,7 +513,7 @@ virBitmapParse(const char *str,
{
g_autoptr(virBitmap) tmp = virBitmapNew(bitmapSize);
-if (virBitmapParseInternal(str, tmp, true) < 0)
+if (virBitmapParseInternal(str, tmp, true, false) < 0)
return -1;
*bitmap = g_steal_pointer(&tmp);
@@ -534,7 +542,29 @@ virBitmapParseUnlimited(const char *str)
{
g_autoptr(virBitmap) tmp = virBitmapNew(0);
-if (virBitmapParseInternal(str, tmp, false) < 0)
+if (virBitmapParseInternal(str, tmp, false, false) < 0)
+return NULL;
+
+return g_steal_pointer(&tmp);
+}
+
+
+/**
+ * virBitmapParseUnlimitedAllowEmpty:
+ * @str: points to a string representing a human-readable bitmap
+ *
+ * Just like virBitmapParseUnlimited() except when the input string @str is
+ * empty (or contains just spaces) an empty bitmap is returned instead of an
+ * error.
+ *
+ * Returns @bitmap on success, or NULL in cas of error
+ */
+virBitmap *
+virBitmapParseUnlimitedAllowEmpty(const char *str)
+{
+g_autoptr(virBitmap) tmp = virBitmapNew(0);
+
+if (virBitmapParseInternal(str, tmp, false, true) < 0)
return NULL;
return g_steal_pointer(&tmp);
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index a9cf309884..a9f9d97fd0 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -84,6 +84,9 @@ int virBitmapParse(const char *str,
virBitmap *
virBitmapParseUnlimited(const char *str);
+virBitmap *
+virBitmapParseUnlimitedAllowEmpty(const char *str);
+
virBitmap *virBitmapNewCopy(virBitmap *src) ATTRIBUTE_NONNULL(1);
virBitmap *virBitmapNewData(const void *data, int len) ATTRIBUTE_NONNULL(1);
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index f4fadb7c8a..adc956ca3d 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -705,6 +705,43 @@ test16(const void *opaque G_GNUC_UNUSED)
}
+/* virBitmapParseUnlimitedAllowEmpty */
+static int
+test17(const void *opaque G_GNUC_UNUSED)
+{
+g_autoptr(virBitmap) map1 = NULL;
+g_autoptr(virBitmap) map2 = NULL;
+g_autofree char *map1_str = NULL;
+g_autofree char *map2_str = NULL;
+
+if (!(map1 = virBitmapParseUnlimitedAllowEmpty(NULL))) {
+fprintf(stderr, "Expected success, got failure\n");
+return -1;
+}
+
+if (!(map2 = virBitmapParseUnlimitedAllowEmpty(""))) {
+fprintf(stderr, "Expected success, got failure\n");
+return -1;
+}
+
+if (!virBitmapIsAllClear(map1) ||
+!virBitmapIsAllClear(map2) ||
+!virBitmapEqual(map1, map2)) {
+fprintf(stderr, "empty maps should equal\n");
+return -1;
+}
+
+if (!(map1_str = virBitmapFormat(
[PATCH v2 3/4] virhostcpu: Introduce virHostCPUGetIsolated()
This is a helper that parses /sys/devices/system/cpu/isolated
into a virBitmap. It's going to be needed soon.
Signed-off-by: Michal Privoznik
---
src/libvirt_private.syms | 1 +
src/util/virhostcpu.c| 31 +++
src/util/virhostcpu.h| 1 +
3 files changed, 33 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2c7e4b45d3..0f2d5db883 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2504,6 +2504,7 @@ virHostCPUGetCount;
virHostCPUGetCPUID;
virHostCPUGetHaltPollTime;
virHostCPUGetInfo;
+virHostCPUGetIsolated;
virHostCPUGetKVMMaxVCPUs;
virHostCPUGetMap;
virHostCPUGetMicrocodeVersion;
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 01de69c0d1..b6d1db2302 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1152,6 +1152,37 @@ virHostCPUGetAvailableCPUsBitmap(void)
}
+/**
+ * virHostCPUGetIsolated:
+ * @isolated: returned bitmap of isolated CPUs
+ *
+ * Sets @isolated to point to a bitmap of isolated CPUs (e.g. those passed to
+ * isolcpus= kernel cmdline). If the file doesn't exist, @isolated is set to
+ * NULL and success is returned. If the file does exist but it's empty,
+ * @isolated is set to an empty bitmap an success is returned.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with error reported).
+ */
+int
+virHostCPUGetIsolated(virBitmap **isolated)
+{
+g_autoptr(virBitmap) bitmap = NULL;
+int rc;
+
+rc = virFileReadValueBitmapAllowEmpty(&bitmap, "%s/cpu/isolated",
SYSFS_SYSTEM_PATH);
+if (rc == -2) {
+*isolated = NULL;
+return 0;
+} else if (rc < 0) {
+return -1;
+}
+
+*isolated = g_steal_pointer(&bitmap);
+return 0;
+}
+
+
#if WITH_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT)
/* Get the number of threads per subcore.
diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
index d7e09bff22..1f47634c33 100644
--- a/src/util/virhostcpu.h
+++ b/src/util/virhostcpu.h
@@ -43,6 +43,7 @@ bool virHostCPUHasBitmap(void);
virBitmap *virHostCPUGetPresentBitmap(void);
virBitmap *virHostCPUGetOnlineBitmap(void);
virBitmap *virHostCPUGetAvailableCPUsBitmap(void);
+int virHostCPUGetIsolated(virBitmap **isolated);
int virHostCPUGetCount(void);
int virHostCPUGetThreadsPerSubcore(virArch arch) G_NO_INLINE;
--
2.43.2
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 0/4] qemu: Substract isolcpus from all online affinity
v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/4V7OI5AEGYRN4GFQMQPIN4MYPJNK3NYJ/ diff to v1: - Don't error out on systems where /sys/devices/system/cpu/isolated is unavailable. - Don't error out on systems where /sys/devices/system/cpu/isolated is empty. Both of these resulted in new patches. Michal Prívozník (4): virbitmap: Introduce virBitmapParseUnlimitedAllowEmpty() virfile: Introduce virFileReadValueBitmapAllowEmpty() virhostcpu: Introduce virHostCPUGetIsolated() qemu: Substract isolcpus from all online affinity src/libvirt_private.syms | 3 ++ src/qemu/qemu_process.c | 9 + src/util/virbitmap.c | 40 +--- src/util/virbitmap.h | 3 ++ src/util/virfile.c | 81 ++-- src/util/virfile.h | 2 + src/util/virhostcpu.c| 31 +++ src/util/virhostcpu.h| 1 + tests/virbitmaptest.c| 40 9 files changed, 186 insertions(+), 24 deletions(-) -- 2.43.2 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 4/4] qemu: Substract isolcpus from all online affinity
When starting a domain and there's no vCPU/emulator pinning set,
we query the list of all online physical CPUs and set affinity of
the child process (which eventually becomes QEMU) to that list.
We can't assume libvirtd itself had affinity to all online CPUs
and since affinity of the child process is inherited, we should
fix it afterwards. But that's not necessarily correct. Users
might isolate some physical CPUs and we should avoid touching
them unless explicitly told so (i.e. vCPU/emulator pinning told
us so).
Therefore, when attempting to set affinity to all online CPUs
subtract the isolated ones.
Before this commit:
root@localhost:~# cat /sys/devices/system/cpu/isolated
19,21,23
root@virtlab414:~# taskset -cp $(pgrep qemu)
pid 14835's current affinity list: 0-23
After:
root@virtlab414:~# taskset -cp $(pgrep qemu)
pid 17153's current affinity list: 0-18,20,22
Resolves: https://issues.redhat.com/browse/RHEL-33082
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_process.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index da2b024f92..521598471f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2334,6 +2334,8 @@ qemuProcessDetectIOThreadPIDs(virDomainObj *vm,
static int
qemuProcessGetAllCpuAffinity(virBitmap **cpumapRet)
{
+g_autoptr(virBitmap) isolCpus = NULL;
+
*cpumapRet = NULL;
if (!virHostCPUHasBitmap())
@@ -2342,6 +2344,13 @@ qemuProcessGetAllCpuAffinity(virBitmap **cpumapRet)
if (!(*cpumapRet = virHostCPUGetOnlineBitmap()))
return -1;
+if (virHostCPUGetIsolated(&isolCpus) < 0)
+return -1;
+
+if (isolCpus) {
+virBitmapSubtract(*cpumapRet, isolCpus);
+}
+
return 0;
}
--
2.43.2
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 10/27] util/network: new virFirewallBackend enum
On 4/23/24 6:10 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:18PM -0400, Laine Stump wrote:
(This paragraph is for historical reference only, described only to
avoid confusion of past use of the name with its new use) In a past
life, virFirewallBackend had been a private static in virfirewall.c
that was set at daemon init time, and used to globally (i.e. for all
drivers in the daemon) determine whether to directly execute iptables
commands, or to run them indirectly via the firewalld passthrough
API. This was removed in commit d566cc55, since we decided that using
the firewalld passthrough API is never appropriate.
Now the same enum, virFirewallBackend, is being reintroduced, with a
different meaning and usage pattern. It will be used to pick between
using nftables commands or iptables commands (in either case directly
handled by libvirt, *not* via firewalld). Additionally, rather than
being a static known only within virfirewall.c and applying to all
firewall commands for all drivers, each virFirewall object will have
its own backend setting, which will be set during virFirewallNew() by
the driver who wants to add a firewall rule.
This will allow the nwfilter and network drivers to each have their
own backend setting, even when they coexist in a single unified
daemon. At least as important as that, it will also allow an instance
of the network driver to remove iptables rules that had been added by
a previous instance, and then add nftables rules for the new instance
(in the case that an admin, or possibly an update, switches the driver
backend from iptables to nftable)
Initially, the enum will only have one usable value -
VIR_FIREWALL_BACKEND_IPTABLES, and that will be hardcoded into all
calls to virFirewallNew(). The other enum value (along with a method
of setting it for each driver) will be added later, when it can be
used (when the nftables backend is in the code).
Signed-off-by: Laine Stump
---
src/libvirt_private.syms | 3 +++
src/network/network_iptables.c| 6 +++---
src/nwfilter/nwfilter_ebiptables_driver.c | 16
src/util/virebtables.c| 4 ++--
src/util/virfirewall.c| 16 +++-
src/util/virfirewall.h| 12 +++-
tests/virfirewalltest.c | 20 ++--
7 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 56d43bfdde..489482fd83 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -35,6 +35,11 @@
VIR_LOG_INIT("util.firewall");
+VIR_ENUM_IMPL(virFirewallBackend,
+ VIR_FIREWALL_BACKEND_LAST,
+ "UNSET", /* not yet set */
+ "iptables");
+
snip
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 956bf0e2bf..7f0fee047d 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -34,9 +35,18 @@ typedef enum {
VIR_FIREWALL_LAYER_LAST,
} virFirewallLayer;
-virFirewall *virFirewallNew(void);
+typedef enum {
+VIR_FIREWALL_BACKEND_UNSET,
+VIR_FIREWALL_BACKEND_IPTABLES,
+
+VIR_FIREWALL_BACKEND_LAST,
+} virFirewallBackend;
We're forcing the callers to all pass VIR_FIREWALL_BACKEND_IPTABLES,
That is the case now, but as soon as we add another backend, then it's
possible for it to be UNSET (if the init code that sets it hasn't been
run yet (should never happen), or (and this is more likely) it can't
find either iptables or nft and so the autodetect fails.
so I'm wondering if we actually need to have VIR_FIREWALL_BACKEND_UNSET
at all ? If we eliminate it, then that removes need to check for this
illegal value and report errors I guess.
With regards,
Daniel
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 12/27] network: support setting firewallBackend from network.conf
On 4/23/24 6:17 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:20PM -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 not, it will be left as "unset", which
(once multiple backends are available) will trigger an appropriate
error message the first time we attempt to add a rule.
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 22 +++--
src/network/bridge_driver_conf.c | 40
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/network.conf | 8 +
src/network/test_libvirtd_network.aug.in | 3 ++
tests/networkxml2firewalltest.c | 2 +-
10 files changed, 83 insertions(+), 18 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d89700c6ee..38e4ab84ad 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c
index a2edafa837..9769ee06b5 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)
@@ -73,6 +75,44 @@ 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));
Since the BACKEND enum has "unset" as a valid entry, and you're checking
'backend < 0' here,
Oops. That should have been <= 0, which is something we commonly do in
the conf directory when we don't want to allow, e.g., formatting an
"attr='default' for an attribute that hasn't been set.
this allows the user to explicitly do
firewall_backend="UNSET"
To me this is another reason to eliminate the "UNSET" backend enum value.
+
+} 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;
+
+/* virFindFileInPath() uses g_find_program_in_path(),
+ * which allows absolute paths, and verifies that
+ * the file is executable.
+*/
+if ((iptablesInPath = virFindFileInPath(IPTABLES)))
+cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES;
+
+if (cfg->firewallBackend == VIR_FIREWALL_BACKEND_UNSET)
Rather than checking against "UNSET", this could just be an 'else'
clause, and a fatal error.
in the final version, it is:
if (iptables is found)
backend = iptables
else if (nftables is found)
backend = nftables
if (backend == unset)
INFO("couldn't find a backend"
else
INFO("backend set to %s", backend)
Using a fatal error would mean the virnetworkd will fail to start,
since and journalctl will give the explanation.
If we only report it later at time of first usage, then the app
user will see it, but they're not in a position to fix it. Showing
a failed service looks to give more attention to the admin.
Of course they should not get into this situation in the first
place with a packaged distro, since the distro should have added
deps to pull in either iptables or nftables or both.
Okay, I can do that (I was *kind of* (but not really :-/) following in
the footsteps of the pre-existing networkSetupPrivate
Re: [PATCH v2 13/27] network: framework to call backend-specific function to init private filter chains
On 4/23/24 6:21 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:21PM -0400, Laine Stump wrote:
>> [...]
+static int
+networkFirewallSetupPrivateChains(virFirewallBackend backend,
+ virFirewallLayer layer)
+{
+switch (backend) {
+case VIR_FIREWALL_BACKEND_IPTABLES:
+return iptablesSetupPrivateChains(layer);
+
+case VIR_FIREWALL_BACKEND_UNSET:
+case VIR_FIREWALL_BACKEND_LAST:
+networkFirewallBackendUnsetError();
for _LAST and default: cases, you should use
virReportEnumRangeError(virFirewallBackend, backend)
Right, I forgot about that.
+return -1;
+}
+return 0;
+}
+
With regards,
Daniel
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 26/27] network: prefer the nftables backend over iptables
On 4/23/24 6:40 AM, Daniel P. Berrangé wrote: I wonder if we shouldn't make the default firewall backend be a meson_options.txt parameter. Good idea! If a distro rebases libvirt in their existing release, they probably don't want the firewall backend silently changing as a side effect. A meson option would let them turn on the new behaviour for only new releases. We could make the meson option default to 'nft' though. ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 12/27] network: support setting firewallBackend from network.conf
On Tue, Apr 23, 2024 at 11:48:24AM -0400, Laine Stump wrote:
> On 4/23/24 6:17 AM, Daniel P. Berrangé wrote:
> > On Sun, Apr 21, 2024 at 10:53:20PM -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 not, it will be left as "unset", which
> > > (once multiple backends are available) will trigger an appropriate
> > > error message the first time we attempt to add a rule.
> >
> >
> >
> > >
> > > Signed-off-by: Laine Stump
> > > ---
> > > src/network/bridge_driver.c | 22 +++--
> > > src/network/bridge_driver_conf.c | 40
> > > 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/network.conf | 8 +
> > > src/network/test_libvirtd_network.aug.in | 3 ++
> > > tests/networkxml2firewalltest.c | 2 +-
> > > 10 files changed, 83 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > > index d89700c6ee..38e4ab84ad 100644
> > > --- a/src/network/bridge_driver.c
> > > +++ b/src/network/bridge_driver.c
> >
> > > diff --git a/src/network/bridge_driver_conf.c
> > > b/src/network/bridge_driver_conf.c
> > > index a2edafa837..9769ee06b5 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)
> > > @@ -73,6 +75,44 @@ 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));
> >
> > Since the BACKEND enum has "unset" as a valid entry, and you're checking
> > 'backend < 0' here,
>
> Oops. That should have been <= 0, which is something we commonly do in the
> conf directory when we don't want to allow, e.g., formatting an
> "attr='default' for an attribute that hasn't been set.
>
> > this allows the user to explicitly do
> >
> >firewall_backend="UNSET"
> >
> > To me this is another reason to eliminate the "UNSET" backend enum value.
> >
> > > +
> > > +} 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;
> > > +
> > > +/* virFindFileInPath() uses g_find_program_in_path(),
> > > + * which allows absolute paths, and verifies that
> > > + * the file is executable.
> > > +*/
> > > +if ((iptablesInPath = virFindFileInPath(IPTABLES)))
> > > +cfg->firewallBackend = VIR_FIREWALL_BACKEND_IPTABLES;
> > > +
> > > +if (cfg->firewallBackend == VIR_FIREWALL_BACKEND_UNSET)
> >
> > Rather than checking against "UNSET", this could just be an 'else'
> > clause, and a fatal error.
>
> in the final version, it is:
>
>if (iptables is found)
> backend = iptables
>else if (nftables is found)
> backend = nftables
>
>if (backend == unset)
>INFO("couldn't find a backend"
>el
Re: [PATCH v2 27/27] RFC: spec: change iptables/ebtables from Requires to Recommends, add nftables
On 4/23/24 6:46 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:35PM -0400, Laine Stump wrote:
We really shouldn't be requiring ebtables and iptables any more, since
they don't always need to be used. Likewise, we probably should at
least Recommend nftables, even though it's pretty much always
installed already anyway.
(Changing Requires to Recommends for the nwfilter package is a bit
premature, since it currently will always require iptables and
ebtables to function properly, but changing those to Recommends leads
to a much smaller list of dependent packages removed by "dnf rm
iptables/ebtables").
Signed-off-by: Laine Stump
---
libvirt.spec.in | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 05f7a7e7c0..66b328671d 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -592,7 +592,8 @@ Summary: Network driver plugin for the libvirtd daemon
Requires: libvirt-daemon-common = %{version}-%{release}
Requires: libvirt-libs = %{version}-%{release}
Requires: dnsmasq >= 2.41
-Requires: iptables
+Recommends: iptables
+Recommends: nftables
Or we use a bool expression:
Requires: (iptables or nftables)
which guarantees at least one is present and thus no possibility of
a broken install
TIL.
%description daemon-driver-network
The network driver plugin for the libvirtd daemon, providing
@@ -603,8 +604,8 @@ bridge capabilities.
Summary: Nwfilter driver plugin for the libvirtd daemon
Requires: libvirt-daemon-common = %{version}-%{release}
Requires: libvirt-libs = %{version}-%{release}
-Requires: iptables
-Requires: ebtables
+Recommends: iptables
+Recommends: ebtables
This looks premature since we've not provided an nft backend option
for nwfilter. Thus the only effect of this change is to guarantee
the ability to create a broken instalation
Yeah, I only left it in here because this is an RFC patch, and I was
curious how much it would reduce the side-effect package removals when
someone did "dnf rm ebtables iptables" (a lot, it turns out).
When the time comes though we would do
Requires: (iptables or nftables)
Requires: (ebtables if iptables)
If only specfiles had a way to slap the admin with a rubber chicken "if
iptables"...
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 01/27] util/network: move viriptables.[ch] from util to network directory
On 4/23/24 5:52 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:09PM -0400, Laine Stump wrote:
These functions are only ever used by the network driver, and are so
specific to the network driver's usage of iptables that they likely
won't ever be used elsewhere. The files are renamed to
network_iptables.[ch] to be more in line with driver-specific file
naming conventions.
Signed-off-by: Laine Stump
---
po/POTFILES | 2 +-
src/libvirt_private.syms | 31 ---
src/network/bridge_driver_linux.c | 2 +-
src/network/meson.build | 1 +
.../network_iptables.c} | 7 +++--
.../network_iptables.h} | 2 +-
src/util/meson.build | 1 -
7 files changed, 8 insertions(+), 38 deletions(-)
rename src/{util/viriptables.c => network/network_iptables.c} (99%)
rename src/{util/viriptables.h => network/network_iptables.h} (99%)
Reviewed-by: Daniel P. Berrangé
with one question below...
diff --git a/src/util/viriptables.c b/src/network/network_iptables.c
similarity index 99%
rename from src/util/viriptables.c
rename to src/network/network_iptables.c
index 018021bc1b..bf6e3065f5 100644
--- a/src/util/viriptables.c
+++ b/src/network/network_iptables.c
@@ -1,5 +1,5 @@
/*
- * viriptables.c: helper APIs for managing iptables
+ * network_iptables.c: helper APIs for managing iptables in network driver
*
* Copyright (C) 2007-2014 Red Hat, Inc.
*
@@ -27,13 +27,14 @@
#include
#include "internal.h"
-#include "viriptables.h"
#include "virfirewalld.h"
#include "virerror.h"
#include "virlog.h"
#include "virhash.h"
+#include "virenum.h"
Presume this virenum.h addition is an accident, that should be in a
later commit ?
Yes. I had moved around the order of the patches (actually created this
branch by cherry-picking some patches from the old branch in a very
different order, interspersed with new patches as well as not bring in
several of the old patches). Anyway, this was the result of all that.
Thanks for your close scrutiny!
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 16/27] network: turn on auto-rollback for the rules added for virtual networks
On 4/23/24 6:53 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:24PM -0400, Laine Stump wrote:
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 3a9f409e2a..e61787daec 100644
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -79,7 +79,14 @@ testCommandDryRun(const char *const*args G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED)
{
*status = 0;
-*output = g_strdup("");
+/* if arg[1] is -ae then this is an nft command,
+ * and the caller requested to get the handle
+ * of the newly added object in stdout
+ */
+if (STREQ_NULLABLE(args[1], "-ae"))
+*output = g_strdup("# handle 5309");
+else
+*output = g_strdup("");
Belongs in the later nft tests patch
Ah, yep! Another byproduct of reordering patches (and
splitting/recombining/rewriting others).
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 18/27] util: new functions virFirewallParseXML() and virFirewallFormat()
On 4/23/24 6:59 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:26PM -0400, Laine Stump wrote:
+ */
+int
+virFirewallParseXML(virFirewall **firewall,
+xmlNodePtr node,
+xmlXPathContextPtr ctxt)
+{
[...]
+nargs = virXPathNodeSet("./item", ctxt, &argsNodes);
+if (nargs < 0)
+return -1;
+if (nargs == 0)
+continue;
In an earlier patch you indicated that nargs == 0 was an error
condition we should check and report. How about reporting it
here too, rather than delaying it ?
Makes sense.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 24/27] network: add an nftables backend for network driver's firewall construction
On 4/23/24 7:15 AM, Daniel P. Berrangé wrote:
On Sun, Apr 21, 2024 at 10:53:32PM -0400, Laine Stump wrote:
Support using nftables to setup the firewall for each virtual network,
rather than iptables. The initial implementation of the nftables
backend creates (almost) exactly the same ruleset as the iptables
backend, determined by running the following commands on a host that
has an active virtual network:
iptables-save >iptables.txt
iptables-restore-translate -f iptables.txt
(and the similar ip6tables-save/ip6tables-restore-translate for an
IPv6 network). Correctness of the new backend was checked by comparing
the output of:
nft list ruleset
when the backend is set to iptables and when it is set to nftables.
This page was used as a guide:
https://wiki.nftables.org/wiki-nftables/index.php/Moving_from_iptables_to_nftables
The only differences between the rules created by the nftables backed
vs. the iptables backend (aside from a few inconsequential changes in
display order of some chains/options) are:
1) When we add nftables rules, rather than adding them in the
system-created "filter" and "nat" tables, we add them in a private
table (ie only we should be using it) created by us called "libvirt"
(the system-created "filter" and "nat" tables can't be used because
adding any rules to those tables directly with nft will cause failure
of any legacy application attempting to use iptables when it tries to
list the iptables rules (e.g. "iptables -S").
(NB: in nftables only a single table is required for both nat and
filter rules - the chains for each are differentiated by specifying
different "hook" locations for the toplevel chain of each)
2) nftables doesn't support the "checksum mangle" rule (or any
equivalent functionality) that we have historically added to our
iptables rules, so the nftables rules we add have nothing related to
checksum mangling.
(NB: The result of (2) is that if you a) have a very old guest (RHEL5
era or earlier) and b) that guest is using a virtio-net network
device, and c) the virtio-net device is using vhost packet processing
(the default) then DHCP on the guest will fail. You can work around
this by adding to the XML for the
guest).
There are certainly much better nftables rulesets that could be used
instead of those implemented here, and everything is in place to make
future changes to the rules that are used simple and free of surprises
(e.g. the rules that are added have coresponding "removal" commands
added to the network status so that we will always remove exactly the
rules that were previously added rather than trying to remove the
rules that "this build of libvirt would have added" (which will be
incorrect the first time we run a libvirt with a newly modified
ruleset). For this initial implementation though, I wanted the
nftables rules to be as identical to the iptables rules as possible,
just to make it easier to verify that everything is working.
The backend can be manually chosen using the firewall_backend setting
in /etc/libvirt/network.conf. libvirtd/virtnetworkd will read this
setting when it starts; if there is no explicit setting, it will look
for iptables commands on the host and, if they are found, will select
the iptables backend (this is the default for the sake of 100%
backward compatibility), but if iptables commands aren't found, then
it will use the nftables backend.
(Since libvirt will automatically remove all its previous filter rules
and re-add rules using the current backend setting for all active
networks when it starts up, and the only noticeable change in behavior
between the iptables and nftables backends is that noted in item (2)
above, we could instead decide to make nftables the default backend
rather than iptables - it all depends on how important it is to work
properly on 15 year old guest OSes using DHCP with virtio-net
interfaces).
Signed-off-by: Laine Stump
---
po/POTFILES | 1 +
src/network/bridge_driver_conf.c | 3 +
src/network/bridge_driver_linux.c | 18 +-
src/network/meson.build | 1 +
src/network/network.conf | 17 +-
src/network/network_nftables.c| 940 ++
src/network/network_nftables.h| 28 +
src/util/virfirewall.c| 169 +-
src/util/virfirewall.h| 2 +
9 files changed, 1174 insertions(+), 5 deletions(-)
create mode 100644 src/network/network_nftables.c
create mode 100644 src/network/network_nftables.h
+if (needRollback) {
+virFirewallCmd *rollback = virFirewallAddRollbackCmd(firewall,
fwCmd->layer, NULL);
+const char *handleStart = NULL;
+size_t handleLen = 0;
+g_autofree char *handleStr = NULL;
+g_autofree char *rollbackStr = NULL;
+
+/* Search for "# handle n" in stdout of the nft add command -
+ * that is the handle of the table/rule/chain that will later
+ * need to be deleted.
+ */
What are
Re: [PATCH v2 24/27] network: add an nftables backend for network driver's firewall construction
On Tue, Apr 23, 2024 at 01:27:05PM -0400, Laine Stump wrote: > On 4/23/24 7:15 AM, Daniel P. Berrangé wrote: > > On Sun, Apr 21, 2024 at 10:53:32PM -0400, Laine Stump wrote: > > > Support using nftables to setup the firewall for each virtual network, > > > rather than iptables. The initial implementation of the nftables > > > backend creates (almost) exactly the same ruleset as the iptables > > > backend, determined by running the following commands on a host that > > > has an active virtual network: > > > > > >iptables-save >iptables.txt > > >iptables-restore-translate -f iptables.txt > > > > > > (and the similar ip6tables-save/ip6tables-restore-translate for an > > > IPv6 network). Correctness of the new backend was checked by comparing > > > the output of: > > > > > >nft list ruleset > > > > > > when the backend is set to iptables and when it is set to nftables. > > > > > > This page was used as a guide: > > > > > > > > > https://wiki.nftables.org/wiki-nftables/index.php/Moving_from_iptables_to_nftables > > > > > > The only differences between the rules created by the nftables backed > > > vs. the iptables backend (aside from a few inconsequential changes in > > > display order of some chains/options) are: > > > > > > 1) When we add nftables rules, rather than adding them in the > > > system-created "filter" and "nat" tables, we add them in a private > > > table (ie only we should be using it) created by us called "libvirt" > > > (the system-created "filter" and "nat" tables can't be used because > > > adding any rules to those tables directly with nft will cause failure > > > of any legacy application attempting to use iptables when it tries to > > > list the iptables rules (e.g. "iptables -S"). > > > > > > (NB: in nftables only a single table is required for both nat and > > > filter rules - the chains for each are differentiated by specifying > > > different "hook" locations for the toplevel chain of each) > > > > > > 2) nftables doesn't support the "checksum mangle" rule (or any > > > equivalent functionality) that we have historically added to our > > > iptables rules, so the nftables rules we add have nothing related to > > > checksum mangling. > > > > > > (NB: The result of (2) is that if you a) have a very old guest (RHEL5 > > > era or earlier) and b) that guest is using a virtio-net network > > > device, and c) the virtio-net device is using vhost packet processing > > > (the default) then DHCP on the guest will fail. You can work around > > > this by adding to the XML for the > > > guest). > > > > > > There are certainly much better nftables rulesets that could be used > > > instead of those implemented here, and everything is in place to make > > > future changes to the rules that are used simple and free of surprises > > > (e.g. the rules that are added have coresponding "removal" commands > > > added to the network status so that we will always remove exactly the > > > rules that were previously added rather than trying to remove the > > > rules that "this build of libvirt would have added" (which will be > > > incorrect the first time we run a libvirt with a newly modified > > > ruleset). For this initial implementation though, I wanted the > > > nftables rules to be as identical to the iptables rules as possible, > > > just to make it easier to verify that everything is working. > > > > > > The backend can be manually chosen using the firewall_backend setting > > > in /etc/libvirt/network.conf. libvirtd/virtnetworkd will read this > > > setting when it starts; if there is no explicit setting, it will look > > > for iptables commands on the host and, if they are found, will select > > > the iptables backend (this is the default for the sake of 100% > > > backward compatibility), but if iptables commands aren't found, then > > > it will use the nftables backend. > > > > > > (Since libvirt will automatically remove all its previous filter rules > > > and re-add rules using the current backend setting for all active > > > networks when it starts up, and the only noticeable change in behavior > > > between the iptables and nftables backends is that noted in item (2) > > > above, we could instead decide to make nftables the default backend > > > rather than iptables - it all depends on how important it is to work > > > properly on 15 year old guest OSes using DHCP with virtio-net > > > interfaces). > > > > > > Signed-off-by: Laine Stump > > > --- > > > po/POTFILES | 1 + > > > src/network/bridge_driver_conf.c | 3 + > > > src/network/bridge_driver_linux.c | 18 +- > > > src/network/meson.build | 1 + > > > src/network/network.conf | 17 +- > > > src/network/network_nftables.c| 940 ++ > > > src/network/network_nftables.h| 28 + > > > src/util/virfirewall.c| 169 +- > > > src/util/virfirewall.h| 2 + > > > 9 files changed, 1174 insertio
[PATCH v2 01/20] nodedev: fix mdev add udev event data handling
From: Boris Fiuczynski Two situations will trigger an udev add event: 1) the mdev is created when started (transient) or 2) the mdev was defined and is started In case 1 there is no node object existing and no config data is copied. In case 2 copying the active config data of an existing node object will only copy invalid data. Instead copying the defined config data will store valid data into the newly added node object. Signed-off-by: Boris Fiuczynski Reviewed-by: Jonathon Jongsma Reviewed-by: Marc Hartmayer --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f1e402f8f7f6..4730a5b986ca 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device) objdef = virNodeDeviceObjGetDef(obj); if (is_mdev) -nodeDeviceDefCopyFromMdevctl(def, objdef, false); +nodeDeviceDefCopyFromMdevctl(def, objdef, true); persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj); -- 2.34.1 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 00/20] node_dev_udev: use workerpool and improve nodedev events
When an udev event occurs for a mediated device (mdev) the mdev config data requires an update via mdevctl as the udev event does not contain all config data. This update needs to occur immediately and to be finished before the libvirt nodedev event is issued to keep the API usage reliable. Changelog: v1->v2: + squashed old patches 3 and 17 (comments from Jonathon and Boris) + added r-b's from Jonathon and Boris + worked in comments from Jonathon to old patch 15 + added comment why only one worker can currently be used + added patch `node_device_udev: remove incorrect G_GNUC_UNUSED` RFCv1->v1: + removed some of my own s-o-b's that were accidentally inserted in the RFC + added r-b's from Boris and Jonathon + worked in comments from Boris and Jonathon, but I did not inline "nodeDeviceDefResetMdevActiveConfig" as I'm not sure whether this improves the readability + reworked patch "[RFC PATCH v1 11/15] node_device_udev: Use `stateShutdownPrepare` and `stateShutdownWait`" + reworked patch "node_device_udev: Use a worker pool for processing events and emitting nodedev event" + added patches: - node_device_udev: Move responsibility to release `(init|udev)Thread` to `udevEventDataDispose` - node_device_udev: Fix leak of mdevctlLock, udevThreadCond, and mdevCtlMonitor - node_device_udev: nodeStateShutdownPrepare: Disconnect the signals explicitly - node_device_udev: Pass the driver state as parameter in prepartion for the next commit - node_device_udev: Add support for `g_autoptr` to `udevEventData - node_device_udev: Pass the `udevEventData` via parameter and use refcounting Boris Fiuczynski (3): nodedev: fix mdev add udev event data handling nodedev: immediate update of active config on udev events nodedev: reset active config data on udev remove event Marc Hartmayer (17): node_device_udev: Set @def to NULL node_device_udev: Remove the timeout if the data is disposed node_device_udev: Test for mdevctlTimeout != -1 node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking node_device_udev: Take lock if `driver->privateData` is modified node_device_udev: Add prefix `udev` for udev related data node_device_udev: Inline `udevRemoveOneDevice` node_device_udev: Move responsibility to release `(init|udev)Thread` to `udevEventDataDispose` node_device_udev: Fix leak of mdevctlLock, udevThreadCond, and mdevCtlMonitors node_device_udev: Introduce and use `stateShutdownPrepare` and `stateShutdownWait` node_device_udev: nodeStateShutdownPrepare: Disconnect the signals explicitly node_device_udev: Pass the driver state as parameter in preparation for the next commit node_device_udev: Use a worker pool for processing events and emitting nodedev event node_device_udev: Make the code easier to read node_device_udev: Add support for `g_autoptr` to `udevEventData` node_device_udev: Pass the `udevEventData` via parameter and use refcounting node_device_udev: remove incorrect G_GNUC_UNUSED src/node_device/node_device_driver.h | 5 +- src/util/virmdev.h | 4 + src/conf/node_device_conf.c | 10 +- src/node_device/node_device_driver.c | 28 +- src/node_device/node_device_udev.c | 516 ++- src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 deletions(-) base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 05/20] node_device_udev: Remove the timeout if the data is disposed
Remove the timeout when the udevEventData is disposed, analogous to priv->watch. Reviewed-by: Boris Fiuczynski Reviewed-by: Jonathon Jongsma Signed-off-by: Marc Hartmayer --- src/node_device/node_device_udev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 921f7806afbd..945cfea0851e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,6 +88,9 @@ udevEventDataDispose(void *obj) if (priv->watch != -1) virEventRemoveHandle(priv->watch); +if (priv->mdevctlTimeout > 0) +virEventRemoveTimeout(priv->mdevctlTimeout); + if (!priv->udev_monitor) return; -- 2.34.1 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 07/20] node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking
Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the
locking mechanism and accidentally removed the comment with the reason why the
lock is taken. The reason was to "ensure only a single thread can query mdevctl
at a time", but this reason is no longer valid or maybe it never was. Therefore,
let's remove this lock and add a comment to `mdevCtl` what it protects.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 616f98885a60..c2e6c593709b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -72,8 +72,9 @@ struct _udevEventData {
/* init thread */
virThread *initThread;
-GList *mdevctlMonitors;
+/* Protects @mdevctlMonitors */
virMutex mdevctlLock;
+GList *mdevctlMonitors;
int mdevctlTimeout;
};
@@ -2069,9 +2070,6 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
static void
mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED)
{
-udevEventData *priv = driver->privateData;
-VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
-
if (nodeDeviceUpdateMediatedDevices() < 0)
VIR_WARN("mdevctl failed to update mediated devices");
}
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 02/20] node_device_udev: Set @def to NULL
@def is owned by @obj after adding it the node device object list. As soon as the @obj lock has been released, another thread could free @obj and therefore @def. If now someone accesses @def this would lead to a heap-use-after-free and therefore most likely to a segmentation fault, therefore set @def to NULL after the ownership has moved. While at it, add comments to other code places why @def is set to NULL. Reviewed-by: Boris Fiuczynski Signed-off-by: Marc Hartmayer --- src/node_device/node_device_udev.c | 4 src/test/test_driver.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4730a5b986ca..6613528d0e37 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1588,6 +1588,8 @@ udevAddOneDevice(struct udev_device *device) * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; +/* @def is now owned by @obj */ +def = NULL; virNodeDeviceObjSetPersistent(obj, persistent); virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj); @@ -1983,6 +1985,8 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; +/* @def is now owned by @obj */ +def = NULL; virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetAutostart(obj, true); virNodeDeviceObjSetPersistent(obj, true); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b656..81b1ba4294bd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7656,8 +7656,9 @@ testNodeDeviceMockCreateVport(testDriver *driver, if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; -virNodeDeviceObjSetSkipUpdateCaps(obj, true); +/* @def is now owned by @obj */ def = NULL; +virNodeDeviceObjSetSkipUpdateCaps(obj, true); objdef = virNodeDeviceObjGetDef(obj); event = virNodeDeviceEventLifecycleNew(objdef->name, -- 2.34.1 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 06/20] node_device_udev: Test for mdevctlTimeout != -1
It is done a little differently everywhere in libvirt, but most common is to
test for != -1.
Reviewed-by: Boris Fiuczynski
Reviewed-by: Jonathon Jongsma
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 945cfea0851e..616f98885a60 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -88,7 +88,7 @@ udevEventDataDispose(void *obj)
if (priv->watch != -1)
virEventRemoveHandle(priv->watch);
-if (priv->mdevctlTimeout > 0)
+if (priv->mdevctlTimeout != -1)
virEventRemoveTimeout(priv->mdevctlTimeout);
if (!priv->udev_monitor)
@@ -139,6 +139,7 @@ udevEventDataNew(void)
return NULL;
}
+ret->mdevctlTimeout = -1;
ret->watch = -1;
return ret;
}
@@ -2082,7 +2083,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void
*opaque)
udevEventData *priv = opaque;
virThread thread;
-if (priv->mdevctlTimeout > 0) {
+if (priv->mdevctlTimeout != -1) {
virEventRemoveTimeout(priv->mdevctlTimeout);
priv->mdevctlTimeout = -1;
}
@@ -2192,7 +2193,7 @@ scheduleMdevctlUpdate(udevEventData *data,
bool force)
{
if (!force) {
-if (data->mdevctlTimeout > 0)
+if (data->mdevctlTimeout != -1)
virEventRemoveTimeout(data->mdevctlTimeout);
data->mdevctlTimeout = virEventAddTimeout(100,
launchMdevctlUpdateThread,
data, NULL);
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 04/20] nodedev: reset active config data on udev remove event
From: Boris Fiuczynski
When a mdev device is destroyed or stopped the udev remove event
handling needs to reset the active config data of the node object
representing a persisted mdev.
Reviewed-by: Marc Hartmayer
Reviewed-by: Jonathon Jongsma
Signed-off-by: Boris Fiuczynski
---
src/node_device/node_device_driver.h | 3 +++
src/util/virmdev.h | 4
src/conf/node_device_conf.c | 10 ++
src/node_device/node_device_driver.c | 13 +
src/node_device/node_device_udev.c | 1 +
src/util/virmdev.c | 20
src/libvirt_private.syms | 2 ++
7 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.h
b/src/node_device/node_device_driver.h
index b3bc4b2e96ed..f195cfef9d49 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -197,3 +197,6 @@ int
nodeDeviceUpdate(virNodeDevice *dev,
const char *xmlDesc,
unsigned int flags);
+
+void
+nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def);
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 853041ac0619..e8e69040e5d4 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig {
size_t nattributes;
};
+void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config);
+void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig,
virMediatedDeviceConfigFree);
+
typedef struct _virMediatedDeviceType virMediatedDeviceType;
struct _virMediatedDeviceType {
char *id;
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5cfbd6a7eb72..897c67d6af91 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
g_free(data->sg.path);
break;
case VIR_NODE_DEV_CAP_MDEV:
-g_free(data->mdev.defined_config.type);
-g_free(data->mdev.active_config.type);
g_free(data->mdev.uuid);
-for (i = 0; i < data->mdev.defined_config.nattributes; i++)
-virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
-g_free(data->mdev.defined_config.attributes);
-for (i = 0; i < data->mdev.active_config.nattributes; i++)
-virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
-g_free(data->mdev.active_config.attributes);
+virMediatedDeviceConfigClear(&data->mdev.defined_config);
+virMediatedDeviceConfigClear(&data->mdev.active_config);
g_free(data->mdev.parent_addr);
break;
case VIR_NODE_DEV_CAP_CSS_DEV:
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index d99b48138ebf..f623339dc973 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
}
+/* A mediated device definition contains data from mdevctl about the active
+ * device. When the device is deactivated the active configuration data needs
+ * to be removed. */
+void
+nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def)
+{
+if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV)
+return;
+
+virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config);
+}
+
+
int
nodeDeviceSetAutostart(virNodeDevice *device,
int autostart)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index e272d9f1ea2b..921f7806afbd 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1464,6 +1464,7 @@ udevRemoveOneDeviceSysPath(const char *path)
if (virNodeDeviceObjIsPersistent(obj)) {
VIR_FREE(def->sysfs_path);
virNodeDeviceObjSetActive(obj, false);
+nodeDeviceDefResetMdevActiveConfig(def);
} else {
VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
def->name, path);
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 992f3eb1b74c..6ecdbdf0ab77 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -516,6 +516,26 @@ void virMediatedDeviceAttrFree(virMediatedDeviceAttr *attr)
g_free(attr);
}
+void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config)
+{
+if (!config)
+return;
+
+virMediatedDeviceConfigClear(config);
+g_free(config);
+}
+
+void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config)
+{
+size_t i = 0;
+
+g_clear_pointer(&config->type, g_free);
+for (i = 0; i < config->nattributes; i++)
+virMediatedDeviceAttrFree(config->attributes[i]);
+config->nattributes = 0;
+g_clear_pointer(&config->attributes, g_free);
+}
+
#define MDEV_BUS_DIR "/sys/class/mdev_bus"
diff --git a/src/libvirt_private.syms b/src/libvirt_private.
[PATCH v2 08/20] node_device_udev: Take lock if `driver->privateData` is modified
Since @driver->privateData is modified take the lock.
Reviewed-by: Boris Fiuczynski
Reviewed-by: Jonathon Jongsma
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index c2e6c593709b..ee96a8a6c92b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -2232,7 +2232,9 @@ mdevctlEventHandleCallback(GFileMonitor *monitor
G_GNUC_UNUSED,
* configuration change, try to coalesce these changes by waiting for the
* CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the
* signal if that event never comes */
-scheduleMdevctlUpdate(priv, (event_type ==
G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT));
+VIR_WITH_OBJECT_LOCK_GUARD(priv) {
+scheduleMdevctlUpdate(priv, (event_type ==
G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT));
+}
}
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 03/20] nodedev: immediate update of active config on udev events
From: Boris Fiuczynski
When an udev add, change or remove event occurs the mdev active config data
requires an update via mdevctl as the udev does not contain all config data.
This update needs to occur immediately and to be finished before the libvirt
nodedev event is issued to keep the API usage reliable.
After this change, scheduleMdevctlUpdate call is already called in
`udevAddOneDevice` and can therefore be removed in `udevHandleOneDevice`.
Signed-off-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 6613528d0e37..e272d9f1ea2b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1440,9 +1440,6 @@ udevGetDeviceDetails(struct udev_device *device,
}
-static void scheduleMdevctlUpdate(udevEventData *data, bool force);
-
-
static int
udevRemoveOneDeviceSysPath(const char *path)
{
@@ -1475,7 +1472,8 @@ udevRemoveOneDeviceSysPath(const char *path)
virNodeDeviceObjEndAPI(&obj);
/* cannot check for mdev_types since they have already been removed */
-scheduleMdevctlUpdate(driver->privateData, false);
+if (nodeDeviceUpdateMediatedDevices() < 0)
+VIR_WARN("mdevctl failed to update mediated devices");
virObjectEventStateQueue(driver->nodeDeviceEventState, event);
return 0;
@@ -1535,6 +1533,7 @@ udevSetParent(struct udev_device *device,
static int
udevAddOneDevice(struct udev_device *device)
{
+g_autofree char *sysfs_path = NULL;
virNodeDeviceDef *def = NULL;
virNodeDeviceObj *obj = NULL;
virNodeDeviceDef *objdef;
@@ -1549,6 +1548,9 @@ udevAddOneDevice(struct udev_device *device)
def = g_new0(virNodeDeviceDef, 1);
def->sysfs_path = g_strdup(udev_device_get_syspath(device));
+/* Create a copy of sysfs_path so it can be safely accessed, even without
+ * holding the @obj lock during the VIR_WARN(...) call at the end. */
+sysfs_path = g_strdup(def->sysfs_path);
udevGetStringProperty(device, "DRIVER", &def->driver);
@@ -1605,8 +1607,13 @@ udevAddOneDevice(struct udev_device *device)
has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES);
virNodeDeviceObjEndAPI(&obj);
-if (has_mdev_types)
-scheduleMdevctlUpdate(driver->privateData, false);
+/* The added mdev needs an immediate active config update before the event
+ * is issued so that full device information is available at the time that
+ * the 'created' event is emitted. */
+if ((has_mdev_types || is_mdev) && (nodeDeviceUpdateMediatedDevices() <
0)) {
+VIR_WARN("Update of mediated device %s failed",
+ NULLSTR_EMPTY(sysfs_path));
+}
ret = 0;
@@ -1758,19 +1765,12 @@ nodeStateCleanup(void)
static int
udevHandleOneDevice(struct udev_device *device)
{
-virNodeDevCapType dev_cap_type;
const char *action = udev_device_get_action(device);
VIR_DEBUG("udev action: '%s': %s", action,
udev_device_get_syspath(device));
-if (STREQ(action, "add") || STREQ(action, "change")) {
-int ret = udevAddOneDevice(device);
-if (ret == 0 &&
-udevGetDeviceType(device, &dev_cap_type) == 0 &&
-dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
-scheduleMdevctlUpdate(driver->privateData, false);
-return ret;
-}
+if (STREQ(action, "add") || STREQ(action, "change"))
+return udevAddOneDevice(device);
if (STREQ(action, "remove"))
return udevRemoveOneDevice(device);
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 11/20] node_device_udev: Move responsibility to release `(init|udev)Thread` to `udevEventDataDispose`
Everything is released in `udevEventDataDispose` except for the threads, change
this as this makes the code easier to read as it can be simplified a little.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 0ad83efc86a8..9aab4340fa2d 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -86,12 +86,16 @@ udevEventDataDispose(void *obj)
struct udev *udev = NULL;
udevEventData *priv = obj;
+g_clear_pointer(&priv->initThread, g_free);
+
if (priv->watch != -1)
virEventRemoveHandle(priv->watch);
if (priv->mdevctlTimeout != -1)
virEventRemoveTimeout(priv->mdevctlTimeout);
+g_clear_pointer(&priv->udevThread, g_free);
+
if (!priv->udev_monitor)
return;
@@ -1730,14 +1734,10 @@ nodeStateCleanup(void)
priv->udevThreadQuit = true;
virCondSignal(&priv->udevThreadCond);
}
-if (priv->initThread) {
+if (priv->initThread)
virThreadJoin(priv->initThread);
-g_clear_pointer(&priv->initThread, g_free);
-}
-if (priv->udevThread) {
+if (priv->udevThread)
virThreadJoin(priv->udevThread);
-g_clear_pointer(&priv->udevThread, g_free);
-}
}
virObjectUnref(priv);
@@ -2328,7 +2328,6 @@ nodeStateInitialize(bool privileged,
"udev-event", false, NULL) < 0) {
virReportSystemError(errno, "%s",
_("failed to create udev handler thread"));
-g_clear_pointer(&priv->udevThread, g_free);
goto unlock;
}
@@ -2360,7 +2359,6 @@ nodeStateInitialize(bool privileged,
"nodedev-init", false, udev) < 0) {
virReportSystemError(errno, "%s",
_("failed to create udev enumerate thread"));
-g_clear_pointer(&priv->initThread, g_free);
goto cleanup;
}
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 10/20] node_device_udev: Inline `udevRemoveOneDevice`
Inline `udevRemoveOneDevice` as it's used only once.
Reviewed-by: Boris Fiuczynski
Reviewed-by: Jonathon Jongsma
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 5be09aeb23d6..0ad83efc86a8 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1485,16 +1485,6 @@ udevRemoveOneDeviceSysPath(const char *path)
return 0;
}
-
-static int
-udevRemoveOneDevice(struct udev_device *device)
-{
-const char *path = udev_device_get_syspath(device);
-
-return udevRemoveOneDeviceSysPath(path);
-}
-
-
static int
udevSetParent(struct udev_device *device,
virNodeDeviceDef *def)
@@ -1778,8 +1768,11 @@ udevHandleOneDevice(struct udev_device *device)
if (STREQ(action, "add") || STREQ(action, "change"))
return udevAddOneDevice(device);
-if (STREQ(action, "remove"))
-return udevRemoveOneDevice(device);
+if (STREQ(action, "remove")) {
+const char *path = udev_device_get_syspath(device);
+
+return udevRemoveOneDeviceSysPath(path);
+}
if (STREQ(action, "move")) {
const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD");
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 14/20] node_device_udev: nodeStateShutdownPrepare: Disconnect the signals explicitly
The documentation of gobject signals reads:
"If you are connecting handlers to signals and using a GObject instance as your
signal handler user data, you should remember to pair calls to
g_signal_connect() with calls to g_signal_handler_disconnect() or
g_signal_handlers_disconnect_by_func(). While signal handlers are automatically
disconnected when the object emitting the signal is finalised..." [1]
This means that the signal handlers are automatically disconnected as soon as
the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`.
But this also means that it's possible that new work is tried to be scheduled
for the workerpool by the `mdevctlEventHandleCallback` (main thread context)
even if the workerpool has already been stopped by `nodeStateShutdownWait`. To
fully understand this, it's important to know that the main loop of the main
thread is still running for some time even after `nodeStateShutdownPrepare` has
been called. Let's avoid this situation by explicitly disconnect the signals
during `nodeStateShutdownPrepare`, which is called in the main thread, so that
no new work is attempted to be scheduled for the worker pool.
[1]
https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handlers
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index b2c5d11acbdf..2c9c7466da4a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -2226,6 +2226,12 @@ nodeStateShutdownPrepare(void)
if (!priv)
return 0;
+VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) {
+GList *tmp;
+for (tmp = priv->mdevctlMonitors; tmp; tmp = tmp->next)
+g_signal_handlers_disconnect_by_data(tmp->data, priv);
+}
+
VIR_WITH_OBJECT_LOCK_GUARD(priv) {
if (priv->mdevctlTimeout != -1) {
virEventRemoveTimeout(priv->mdevctlTimeout);
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 13/20] node_device_udev: Introduce and use `stateShutdownPrepare` and `stateShutdownWait`
Introduce and use the driver functions for the node state shutdown preparation
and wait. As they're also called in the error/cleanup path of
`nodeStateInitialize`, they must be written in a way, that they can safely be
executed even if not everything is initialized.
In the next commit, these functions will be extended.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 84 ++
1 file changed, 63 insertions(+), 21 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 906b16778eef..b2c5d11acbdf 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -92,12 +92,6 @@ udevEventDataDispose(void *obj)
g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors),
g_object_unref);
}
-if (priv->watch != -1)
-virEventRemoveHandle(priv->watch);
-
-if (priv->mdevctlTimeout != -1)
-virEventRemoveTimeout(priv->mdevctlTimeout);
-
g_clear_pointer(&priv->udevThread, g_free);
if (priv->udev_monitor) {
@@ -1723,24 +1717,10 @@ udevPCITranslateDeinit(void)
static int
nodeStateCleanup(void)
{
-udevEventData *priv = NULL;
-
if (!driver)
return -1;
-priv = driver->privateData;
-if (priv) {
-VIR_WITH_OBJECT_LOCK_GUARD(priv) {
-priv->udevThreadQuit = true;
-virCondSignal(&priv->udevThreadCond);
-}
-if (priv->initThread)
-virThreadJoin(priv->initThread);
-if (priv->udevThread)
-virThreadJoin(priv->udevThread);
-}
-
-virObjectUnref(priv);
+virObjectUnref(driver->privateData);
virObjectUnref(driver->nodeDeviceEventState);
virNodeDeviceObjListFree(driver->devs);
@@ -2231,6 +2211,64 @@ mdevctlEventHandleCallback(GFileMonitor *monitor
G_GNUC_UNUSED,
}
+/* Note: It must be safe to call this function even if the driver was not
+ * successfully initialized. This must be considered when changing this
+ * function. */
+static int
+nodeStateShutdownPrepare(void)
+{
+udevEventData *priv = NULL;
+
+if (!driver)
+return 0;
+
+priv = driver->privateData;
+if (!priv)
+return 0;
+
+VIR_WITH_OBJECT_LOCK_GUARD(priv) {
+if (priv->mdevctlTimeout != -1) {
+virEventRemoveTimeout(priv->mdevctlTimeout);
+priv->mdevctlTimeout = -1;
+}
+
+if (priv->watch) {
+virEventRemoveHandle(priv->watch);
+priv->watch = -1;
+}
+
+priv->udevThreadQuit = true;
+virCondSignal(&priv->udevThreadCond);
+}
+return 0;
+}
+
+
+/* Note: It must be safe to call this function even if the driver was not
+ * successfully initialized. This must be considered when changing this
+ * function. */
+static int
+nodeStateShutdownWait(void)
+{
+udevEventData *priv = NULL;
+
+if (!driver)
+return 0;
+
+priv = driver->privateData;
+if (!priv)
+return 0;
+
+VIR_WITH_OBJECT_LOCK_GUARD(priv) {
+if (priv->initThread)
+virThreadJoin(priv->initThread);
+if (priv->udevThread)
+virThreadJoin(priv->udevThread);
+}
+return 0;
+}
+
+
static int
nodeStateInitialize(bool privileged,
const char *root,
@@ -2365,6 +2403,8 @@ nodeStateInitialize(bool privileged,
return VIR_DRV_STATE_INIT_COMPLETE;
cleanup:
+nodeStateShutdownPrepare();
+nodeStateShutdownWait();
nodeStateCleanup();
return VIR_DRV_STATE_INIT_ERROR;
@@ -2430,6 +2470,8 @@ static virStateDriver udevStateDriver = {
.stateInitialize = nodeStateInitialize, /* 0.7.3 */
.stateCleanup = nodeStateCleanup, /* 0.7.3 */
.stateReload = nodeStateReload, /* 0.7.3 */
+.stateShutdownPrepare = nodeStateShutdownPrepare, /* 10.3.0 */
+.stateShutdownWait = nodeStateShutdownWait, /* 10.3.0 */
};
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 12/20] node_device_udev: Fix leak of mdevctlLock, udevThreadCond, and mdevCtlMonitors
Even if `priv->udev_monitor` was never initialized, the mdevctlLock, udevThread
were. Therefore let's match the order of releasing the resources the order of
allocating the resources in `nodeStateInitialize`.
In addition, use `g_steal_pointer` in `g_list_free_full`.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 9aab4340fa2d..906b16778eef 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -88,6 +88,10 @@ udevEventDataDispose(void *obj)
g_clear_pointer(&priv->initThread, g_free);
+VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) {
+g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors),
g_object_unref);
+}
+
if (priv->watch != -1)
virEventRemoveHandle(priv->watch);
@@ -96,16 +100,12 @@ udevEventDataDispose(void *obj)
g_clear_pointer(&priv->udevThread, g_free);
-if (!priv->udev_monitor)
-return;
-
-udev = udev_monitor_get_udev(priv->udev_monitor);
-udev_monitor_unref(priv->udev_monitor);
-udev_unref(udev);
-
-VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) {
-g_list_free_full(priv->mdevctlMonitors, g_object_unref);
+if (priv->udev_monitor) {
+udev = udev_monitor_get_udev(priv->udev_monitor);
+udev_monitor_unref(priv->udev_monitor);
+udev_unref(udev);
}
+
virMutexDestroy(&priv->mdevctlLock);
virCondDestroy(&priv->udevThreadCond);
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 09/20] node_device_udev: Add prefix `udev` for udev related data
The new names make it easier to understand the purpose of the data.
Reviewed-by: Boris Fiuczynski
Reviewed-by: Jonathon Jongsma
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 48 +++---
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index ee96a8a6c92b..5be09aeb23d6 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -63,11 +63,11 @@ struct _udevEventData {
struct udev_monitor *udev_monitor;
int watch;
-/* Thread data */
-virThread *th;
-virCond threadCond;
-bool threadQuit;
-bool dataReady;
+/* Udev thread data */
+virThread *udevThread;
+virCond udevThreadCond;
+bool udevThreadQuit;
+bool udevDataReady;
/* init thread */
virThread *initThread;
@@ -104,7 +104,7 @@ udevEventDataDispose(void *obj)
}
virMutexDestroy(&priv->mdevctlLock);
-virCondDestroy(&priv->threadCond);
+virCondDestroy(&priv->udevThreadCond);
}
@@ -130,7 +130,7 @@ udevEventDataNew(void)
if (!(ret = virObjectLockableNew(udevEventDataClass)))
return NULL;
-if (virCondInit(&ret->threadCond) < 0) {
+if (virCondInit(&ret->udevThreadCond) < 0) {
virObjectUnref(ret);
return NULL;
}
@@ -1737,16 +1737,16 @@ nodeStateCleanup(void)
priv = driver->privateData;
if (priv) {
VIR_WITH_OBJECT_LOCK_GUARD(priv) {
-priv->threadQuit = true;
-virCondSignal(&priv->threadCond);
+priv->udevThreadQuit = true;
+virCondSignal(&priv->udevThreadCond);
}
if (priv->initThread) {
virThreadJoin(priv->initThread);
g_clear_pointer(&priv->initThread, g_free);
}
-if (priv->th) {
-virThreadJoin(priv->th);
-g_clear_pointer(&priv->th, g_free);
+if (priv->udevThread) {
+virThreadJoin(priv->udevThread);
+g_clear_pointer(&priv->udevThread, g_free);
}
}
@@ -1855,15 +1855,15 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED)
/* continue rather than break from the loop on non-fatal errors */
while (1) {
VIR_WITH_OBJECT_LOCK_GUARD(priv) {
-while (!priv->dataReady && !priv->threadQuit) {
-if (virCondWait(&priv->threadCond, &priv->parent.lock)) {
+while (!priv->udevDataReady && !priv->udevThreadQuit) {
+if (virCondWait(&priv->udevThreadCond, &priv->parent.lock)) {
virReportSystemError(errno, "%s",
_("handler failed to wait on
condition"));
return;
}
}
-if (priv->threadQuit)
+if (priv->udevThreadQuit)
return;
errno = 0;
@@ -1894,7 +1894,7 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED)
* after the udev_monitor_receive_device wouldn't help much
* due to event mgmt and scheduler timing. */
VIR_WITH_OBJECT_LOCK_GUARD(priv) {
-priv->dataReady = false;
+priv->udevDataReady = false;
}
continue;
@@ -1921,11 +1921,11 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED,
VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
if (!udevEventMonitorSanityCheck(priv, fd))
-priv->threadQuit = true;
+priv->udevThreadQuit = true;
else
-priv->dataReady = true;
+priv->udevDataReady = true;
-virCondSignal(&priv->threadCond);
+virCondSignal(&priv->udevThreadCond);
}
@@ -2035,8 +2035,8 @@ nodeStateInitializeEnumerate(void *opaque)
VIR_WITH_OBJECT_LOCK_GUARD(priv) {
ignore_value(virEventRemoveHandle(priv->watch));
priv->watch = -1;
-priv->threadQuit = true;
-virCondSignal(&priv->threadCond);
+priv->udevThreadQuit = true;
+virCondSignal(&priv->udevThreadCond);
}
goto cleanup;
@@ -2330,12 +2330,12 @@ nodeStateInitialize(bool privileged,
udev_monitor_set_receive_buffer_size(priv->udev_monitor,
128 * 1024 * 1024);
-priv->th = g_new0(virThread, 1);
-if (virThreadCreateFull(priv->th, true, udevEventHandleThread,
+priv->udevThread = g_new0(virThread, 1);
+if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread,
"udev-event", false, NULL) < 0) {
virReportSystemError(errno, "%s",
_("failed to create udev handler thread"));
-g_clear_pointer(&priv->th, g_free);
+g_clear_pointer(&priv->udevThread, g_free);
goto unlock;
}
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-l
[PATCH v2 20/20] node_device_udev: remove incorrect G_GNUC_UNUSED
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 14d44d5bcd0e..cc725997a39e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -791,7 +791,7 @@ udevGetSCSIType(virNodeDeviceDef *def G_GNUC_UNUSED,
static int
-udevProcessSCSIDevice(struct udev_device *device G_GNUC_UNUSED,
+udevProcessSCSIDevice(struct udev_device *device,
virNodeDeviceDef *def)
{
int ret = -1;
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 15/20] node_device_udev: Pass the driver state as parameter in preparation for the next commit
It's better practice for all functions called by the threads to pass the driver
via parameter and not global variables. Easier to test and cleaner.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_driver.h | 2 +-
src/node_device/node_device_driver.c | 6 +--
src/node_device/node_device_udev.c | 73
3 files changed, 45 insertions(+), 36 deletions(-)
diff --git a/src/node_device/node_device_driver.h
b/src/node_device/node_device_driver.h
index f195cfef9d49..2781ad136d68 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -147,7 +147,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
bool defined);
int
-nodeDeviceUpdateMediatedDevices(void);
+nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *driver);
void
nodeDeviceGenerateName(virNodeDeviceDef *def,
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index f623339dc973..59c5f9b417a4 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1887,7 +1887,7 @@ removeMissingPersistentMdev(virNodeDeviceObj *obj,
int
-nodeDeviceUpdateMediatedDevices(void)
+nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver)
{
g_autofree virNodeDeviceDef **defs = NULL;
g_autofree virNodeDeviceDef **act_defs = NULL;
@@ -1911,7 +1911,7 @@ nodeDeviceUpdateMediatedDevices(void)
/* Any mdevs that were previously defined but were not returned in the
* latest mdevctl query should be removed from the device list */
data.defs = defs;
-virNodeDeviceObjListForEachRemove(driver->devs,
+virNodeDeviceObjListForEachRemove(node_driver->devs,
removeMissingPersistentMdev, &data);
for (i = 0; i < data.ndefs; i++)
@@ -2374,7 +2374,7 @@ nodeDeviceUpdate(virNodeDevice *device,
cleanup:
virNodeDeviceObjEndAPI(&obj);
if (updated)
-nodeDeviceUpdateMediatedDevices();
+nodeDeviceUpdateMediatedDevices(driver);
return ret;
}
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 2c9c7466da4a..4f8dae3f85c8 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -361,7 +361,8 @@ udevTranslatePCIIds(unsigned int vendor,
static int
-udevProcessPCI(struct udev_device *device,
+udevProcessPCI(virNodeDeviceDriverState *driver_state,
+ struct udev_device *device,
virNodeDeviceDef *def)
{
virNodeDevCapPCIDev *pci_dev = &def->caps->data.pci_dev;
@@ -372,8 +373,8 @@ udevProcessPCI(struct udev_device *device,
char *p;
bool privileged = false;
-VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) {
-privileged = driver->privileged;
+VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) {
+privileged = driver_state->privileged;
}
pci_dev->klass = -1;
@@ -1391,12 +1392,13 @@ udevGetDeviceType(struct udev_device *device,
static int
-udevGetDeviceDetails(struct udev_device *device,
+udevGetDeviceDetails(virNodeDeviceDriverState *driver_state,
+ struct udev_device *device,
virNodeDeviceDef *def)
{
switch (def->caps->data.type) {
case VIR_NODE_DEV_CAP_PCI_DEV:
-return udevProcessPCI(device, def);
+return udevProcessPCI(driver_state, device, def);
case VIR_NODE_DEV_CAP_USB_DEV:
return udevProcessUSBDevice(device, def);
case VIR_NODE_DEV_CAP_USB_INTERFACE:
@@ -1444,13 +1446,14 @@ udevGetDeviceDetails(struct udev_device *device,
static int
-udevRemoveOneDeviceSysPath(const char *path)
+udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state,
+ const char *path)
{
virNodeDeviceObj *obj = NULL;
virNodeDeviceDef *def;
virObjectEvent *event = NULL;
-if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) {
+if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs,
path))) {
VIR_DEBUG("Failed to find device to remove that has udev path '%s'",
path);
return -1;
@@ -1471,20 +1474,21 @@ udevRemoveOneDeviceSysPath(const char *path)
} else {
VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
def->name, path);
-virNodeDeviceObjListRemove(driver->devs, obj);
+virNodeDeviceObjListRemove(driver_state->devs, obj);
}
virNodeDeviceObjEndAPI(&obj);
/* cannot check for mdev_types since they have already been removed */
-if (nodeDeviceUpdateMediatedDevices() < 0)
+if (nodeDeviceUpdateMediatedDevices(driver_state) < 0)
VIR_WARN("mdevctl failed to update mediated devices");
-virObjectEventStateQueue(driver->nodeDeviceEventState, event);
+virObjectEventStateQueue(driver_state->nodeDevi
[PATCH v2 18/20] node_device_udev: Add support for `g_autoptr` to `udevEventData`
Use this feature in `udevEventDataNew`.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 73b71607489f..8687be942722 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -78,6 +78,7 @@ struct _udevEventData {
/* Immutable pointer, self-locking APIs */
virThreadPool *workerPool;
};
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(udevEventData, virObjectUnref);
static virClass *udevEventDataClass;
@@ -121,7 +122,7 @@ VIR_ONCE_GLOBAL_INIT(udevEventData);
static udevEventData *
udevEventDataNew(void)
{
-udevEventData *ret = NULL;
+g_autoptr(udevEventData) ret = NULL;
if (udevEventDataInitialize() < 0)
return NULL;
@@ -129,19 +130,15 @@ udevEventDataNew(void)
if (!(ret = virObjectLockableNew(udevEventDataClass)))
return NULL;
-if (virCondInit(&ret->udevThreadCond) < 0) {
-virObjectUnref(ret);
+if (virCondInit(&ret->udevThreadCond) < 0)
return NULL;
-}
-if (virMutexInit(&ret->mdevctlLock) < 0) {
-virObjectUnref(ret);
+if (virMutexInit(&ret->mdevctlLock) < 0)
return NULL;
-}
ret->mdevctlTimeout = -1;
ret->watch = -1;
-return ret;
+return g_steal_pointer(&ret);
}
typedef enum {
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 17/20] node_device_udev: Make the code easier to read
There is only one case where force is true, therefore let's inline that case.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 1f7123a5fafa..73b71607489f 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -2210,21 +2210,14 @@ mdevctlEnableMonitor(udevEventData *priv)
/* Schedules an mdevctl update for 100ms in the future, canceling any existing
* timeout that may have been set. In this way, multiple update requests in
- * quick succession can be collapsed into a single update. if @force is true,
- * the worker job is submitted immediately. */
+ * quick succession can be collapsed into a single update. */
static void
-scheduleMdevctlUpdate(udevEventData *data,
- bool force)
+scheduleMdevctlUpdate(udevEventData *data)
{
-if (!force) {
-if (data->mdevctlTimeout != -1)
-virEventRemoveTimeout(data->mdevctlTimeout);
-data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate,
- data, NULL);
-return;
-}
-
-submitMdevctlUpdate(-1, data);
+if (data->mdevctlTimeout != -1)
+virEventRemoveTimeout(data->mdevctlTimeout);
+data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate,
+ data, NULL);
}
@@ -2259,7 +2252,11 @@ mdevctlEventHandleCallback(GFileMonitor *monitor
G_GNUC_UNUSED,
* CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the
* signal if that event never comes */
VIR_WITH_OBJECT_LOCK_GUARD(priv) {
-scheduleMdevctlUpdate(priv, (event_type ==
G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT));
+if (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) {
+submitMdevctlUpdate(-1, priv);
+} else {
+scheduleMdevctlUpdate(priv);
+}
}
}
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 16/20] node_device_udev: Use a worker pool for processing events and emitting nodedev event
Use a worker pool for processing the events (e.g. udev, mdevctl config changes)
and the initialization instead of a separate initThread and a mdevctl-thread.
This has the large advantage that we can leverage the job API and now this
thread pool is responsible to do all the "costly-work" and emitting the libvirt
nodedev events.
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_driver.c | 9 +-
src/node_device/node_device_udev.c | 241 +++
src/test/test_driver.c | 8 +-
3 files changed, 185 insertions(+), 73 deletions(-)
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index 59c5f9b417a4..a51537d87ceb 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1421,10 +1421,11 @@ nodeDeviceDestroy(virNodeDevicePtr device)
goto cleanup;
/* Because we're about to release the lock and thus run into a race
- * possibility (however improbable) with a udevAddOneDevice change
- * event which would essentially free the existing @def (obj->def) and
- * replace it with something new, we need to grab the parent field
- * and then find the parent obj in order to manage the vport */
+ * possibility (however improbable) with a
+ * processNodeDeviceAddAndChangeEvent change event which would
+ * essentially free the existing @def (obj->def) and replace it with
+ * something new, we need to grab the parent field and then find the
+ * parent obj in order to manage the vport */
parent = g_strdup(def->parent);
virNodeDeviceObjEndAPI(&obj);
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 4f8dae3f85c8..1f7123a5fafa 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -43,6 +43,7 @@
#include "virnetdev.h"
#include "virmdev.h"
#include "virutil.h"
+#include "virthreadpool.h"
#include "configmake.h"
@@ -69,13 +70,13 @@ struct _udevEventData {
bool udevThreadQuit;
bool udevDataReady;
-/* init thread */
-virThread *initThread;
-
/* Protects @mdevctlMonitors */
virMutex mdevctlLock;
GList *mdevctlMonitors;
int mdevctlTimeout;
+
+/* Immutable pointer, self-locking APIs */
+virThreadPool *workerPool;
};
static virClass *udevEventDataClass;
@@ -86,8 +87,6 @@ udevEventDataDispose(void *obj)
struct udev *udev = NULL;
udevEventData *priv = obj;
-g_clear_pointer(&priv->initThread, g_free);
-
VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) {
g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors),
g_object_unref);
}
@@ -100,6 +99,8 @@ udevEventDataDispose(void *obj)
udev_unref(udev);
}
+g_clear_pointer(&priv->workerPool, virThreadPoolFree);
+
virMutexDestroy(&priv->mdevctlLock);
virCondDestroy(&priv->udevThreadCond);
@@ -143,6 +144,66 @@ udevEventDataNew(void)
return ret;
}
+typedef enum {
+ NODE_DEVICE_EVENT_INIT = 0,
+ NODE_DEVICE_EVENT_UDEV_ADD,
+ NODE_DEVICE_EVENT_UDEV_REMOVE,
+ NODE_DEVICE_EVENT_UDEV_CHANGE,
+ NODE_DEVICE_EVENT_UDEV_MOVE,
+ NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED,
+
+ NODE_DEVICE_EVENT_LAST
+} nodeDeviceEventType;
+
+struct _nodeDeviceEvent {
+nodeDeviceEventType eventType;
+void *data;
+virFreeCallback dataFreeFunc;
+};
+typedef struct _nodeDeviceEvent nodeDeviceEvent;
+
+static void
+nodeDeviceEventFree(nodeDeviceEvent *event)
+{
+if (!event)
+return;
+
+if (event->dataFreeFunc)
+event->dataFreeFunc(event->data);
+g_free(event);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(nodeDeviceEvent, nodeDeviceEventFree);
+
+ /**
+ * nodeDeviceEventSubmit:
+ * @eventType: the event to be processed
+ * @data: additional data for the event processor (the pointer is stolen and
it
+ *will be properly freed using @dataFreeFunc)
+ * @dataFreeFunc: callback to free @data
+ *
+ * Submits @eventType to be processed by the asynchronous event handling
+ * thread.
+ */
+static int nodeDeviceEventSubmit(nodeDeviceEventType eventType, void *data,
virFreeCallback dataFreeFunc)
+{
+nodeDeviceEvent *event = g_new0(nodeDeviceEvent, 1);
+udevEventData *priv = NULL;
+
+if (!driver)
+return -1;
+
+priv = driver->privateData;
+
+event->eventType = eventType;
+event->data = data;
+event->dataFreeFunc = dataFreeFunc;
+if (virThreadPoolSendJob(priv->workerPool, 0, event) < 0) {
+nodeDeviceEventFree(event);
+return -1;
+}
+return 0;
+}
+
static bool
udevHasDeviceProperty(struct udev_device *dev,
@@ -1446,8 +1507,8 @@ udevGetDeviceDetails(virNodeDeviceDriverState
*driver_state,
static int
-udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state,
- const char *path)
+proc
[PATCH v2 19/20] node_device_udev: Pass the `udevEventData` via parameter and use refcounting
Instead of accessing the global `driver` object pass the `udevEventData` as
parameter to the thread handler and watch callback. This has the advantage that:
1. proper refcounting
2. easier to read and test
Reviewed-by: Jonathon Jongsma
Reviewed-by: Boris Fiuczynski
Signed-off-by: Marc Hartmayer
---
src/node_device/node_device_udev.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 8687be942722..14d44d5bcd0e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1863,7 +1863,7 @@ udevEventMonitorSanityCheck(udevEventData *priv,
/**
* udevEventHandleThread
- * @opaque: unused
+ * @opaque: udevEventData
*
* Thread to handle the udevEventHandleCallback processing when udev
* tells us there's a device change for us (add, modify, delete, etc).
@@ -1882,9 +1882,9 @@ udevEventMonitorSanityCheck(udevEventData *priv,
* would still come into play.
*/
static void
-udevEventHandleThread(void *opaque G_GNUC_UNUSED)
+udevEventHandleThread(void *opaque)
{
-udevEventData *priv = driver->privateData;
+g_autoptr(udevEventData) priv = opaque;
struct udev_device *device = NULL;
/* continue rather than break from the loop on non-fatal errors */
@@ -1950,9 +1950,9 @@ static void
udevEventHandleCallback(int watch G_GNUC_UNUSED,
int fd,
int events G_GNUC_UNUSED,
-void *data G_GNUC_UNUSED)
+void *data)
{
-udevEventData *priv = driver->privateData;
+udevEventData *priv = data;
VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
if (!udevEventMonitorSanityCheck(priv, fd))
@@ -2489,7 +2489,7 @@ nodeStateInitialize(bool privileged,
priv->udevThread = g_new0(virThread, 1);
if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread,
-"udev-event", false, NULL) < 0) {
+"udev-event", false, virObjectRef(priv)) < 0) {
virReportSystemError(errno, "%s",
_("failed to create udev handler thread"));
goto unlock;
@@ -2505,7 +2505,7 @@ nodeStateInitialize(bool privileged,
* that appear while the enumeration is taking place. */
priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
VIR_EVENT_HANDLE_READABLE,
-udevEventHandleCallback, NULL, NULL);
+udevEventHandleCallback,
virObjectRef(priv), virObjectUnref);
if (priv->watch == -1)
goto unlock;
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 24/27] network: add an nftables backend for network driver's firewall construction
On 4/23/24 1:42 PM, Daniel P. Berrangé wrote:
On Tue, Apr 23, 2024 at 01:27:05PM -0400, Laine Stump wrote:
[...]
On 4/23/24 7:15 AM, Daniel P. Berrangé wrote:
What are the uniqueness guarantees of handle numbers.
Each table has a monotonically increasing counter (I'd assume at least 32
bits), so it shouldn't be a problem.
But WAIT!!! - While typing this reply I've discovered something new!
Until about 45 minutes ago, I had assumed there was a single systemwide
counter. But based on your question, I asked egarver who told me that there
is a counter for each table. And we create our own tables (called "libvirt",
for both ip and ip6). I just tried manually deleting the libvirt table, and
in that case the counter does start over from 0! :-O
Oh, that's not terrible at all, as the unique constraint is thus
("libvirt", )
which eliminates any risk of us accidentally deleting stuff belonging
to the sysadmin or another app. If someone else creates a table
called 'libvirt' they get to keep all the broken pieces :-)
I can't decide if this is a case of "Ooh! We'd better try to protect against
this!", or "Well, you deliberately broke it, so you get to pick up the
pieces!"
The latter.
Exxxcelllent!
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
RFC: Drop micro part of our release versioning scheme
Hi, Does anyone feel strongly against dropping the "micro" part from libvirt(-python) versions? I think the original idea was to use this number for maintenance releases in -maint branches, but we stopped doing those a long time ago (v3.2.1 was the last and most likely even the only release with micro > 0 since the change in numbering libvirt releases). So the micro part looks quite useless, not to mention I am lazy to type the .0 suffix all the time :-) And if we decide to drop it, what would be the right time? This 10.3 release, the following 10.4 release or should we wait until 11.0? Personally I'd do it just now, but someone might be relying on the numbers and would prefer to know about such change in advance to prepare for it. So perhaps 10.4 or the most conservative 11.0 would be better options. Jirka ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: RFC: Drop micro part of our release versioning scheme
On Wed, Apr 24, 2024 at 08:43:00 +0200, Jiri Denemark wrote: > Hi, > > Does anyone feel strongly against dropping the "micro" part from > libvirt(-python) versions? I think the original idea was to use this > number for maintenance releases in -maint branches, but we stopped doing > those a long time ago (v3.2.1 was the last and most likely even the only > release with micro > 0 since the change in numbering libvirt releases). > So the micro part looks quite useless, not to mention I am lazy to type > the .0 suffix all the time :-) > > And if we decide to drop it, what would be the right time? This 10.3 > release, the following 10.4 release or should we wait until 11.0? > Personally I'd do it just now, but someone might be relying on the > numbers and would prefer to know about such change in advance to prepare > for it. So perhaps 10.4 or the most conservative 11.0 would be better > options. Since we've established that version numbers in libvirt don't mean anything I'd suggest not "waiting for 11.0". Said that I think we should give some notice period, perhaps 2 releases? ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
