[libvirt] [PATCH] libvirt: move default nwfilters to datadir.
libvirt runs correctly without any configuration files, as sensible defaults are used throughout. This commit introduces a layer for nwfilter configuration. This means that default filters are shipped in /usr/share/libvirt/nwfilter/ directory, which can be overridden by things in /etc/libvirt/nwfilter. This is similar to configuration splits as observed in udev, systemd, XDG Base Directory Specification and so on. This will make a distinction and make it obvious if any of the nwfilters are modified by the administrator. --- examples/xml/nwfilter/Makefile.am | 2 +- src/conf/nwfilter_conf.h | 1 + src/nwfilter/nwfilter_driver.c | 18 +++++++++++------- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/examples/xml/nwfilter/Makefile.am b/examples/xml/nwfilter/Makefile.am index ec1e7ee..61f328a 100644 --- a/examples/xml/nwfilter/Makefile.am +++ b/examples/xml/nwfilter/Makefile.am @@ -37,7 +37,7 @@ EXTRA_DIST=$(FILTERS) confdir = $(sysconfdir)/libvirt -NWFILTER_DIR = "$(DESTDIR)$(sysconfdir)/libvirt/nwfilter" +NWFILTER_DIR = "$(DESTDIR)$(datadir)/libvirt/nwfilter" if WITH_NWFILTER install-data-local: diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 6e68ecc..ee427b1 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -577,6 +577,7 @@ struct _virNWFilterDriverState { virNWFilterObjList nwfilters; char *configDir; + char *defaultsDir; bool watchingFirewallD; }; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 8e3db43..2e81dbf 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -173,7 +173,6 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - char *base = NULL; DBusConnection *sysbus = NULL; if (!privileged) @@ -228,17 +227,20 @@ nwfilterStateInitialize(bool privileged, goto error; } - if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0) + if (virAsprintf(&driver->configDir, + SYSCONFDIR "/libvirt/nwfilter") == -1) goto error; - if (virAsprintf(&driver->configDir, - "%s/nwfilter", base) == -1) + if (virNWFilterLoadAllConfigs(&driver->nwfilters, + driver->configDir) < 0) goto error; - VIR_FREE(base); + if (virAsprintf(&driver->defaultsDir, + PKGDATADIR "/nwfilter") == -1) + goto error; if (virNWFilterLoadAllConfigs(&driver->nwfilters, - driver->configDir) < 0) + driver->defaultsDir) < 0) goto error; nwfilterDriverUnlock(); @@ -246,7 +248,6 @@ nwfilterStateInitialize(bool privileged, return 0; error: - VIR_FREE(base); nwfilterDriverUnlock(); nwfilterStateCleanup(); @@ -292,6 +293,8 @@ nwfilterStateReload(void) virNWFilterLoadAllConfigs(&driver->nwfilters, driver->configDir); + virNWFilterLoadAllConfigs(&driver->nwfilters, + driver->defaultsDir); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -345,6 +348,7 @@ nwfilterStateCleanup(void) virNWFilterObjListFree(&driver->nwfilters); VIR_FREE(driver->configDir); + VIR_FREE(driver->defaultsDir); nwfilterDriverUnlock(); } -- 2.1.0
On Tue, Mar 17, 2015 at 06:52:31PM +0000, Dimitri John Ledkov wrote:
libvirt runs correctly without any configuration files, as sensible defaults are used throughout. This commit introduces a layer for nwfilter configuration. This means that default filters are shipped in /usr/share/libvirt/nwfilter/ directory, which can be overridden by things in /etc/libvirt/nwfilter. This is similar to configuration splits as observed in udev, systemd, XDG Base Directory Specification and so on. This will make a distinction and make it obvious if any of the nwfilters are modified by the administrator.
I think this is really not something we want to do. Having multiple directories to store configs in is going to have more fallout than your patch shows. For a start this breaks the ability to delete these filters, since the deletion code assumes they're in a particular directory. Now if the same named filter is present in both locations the delete API does not know which to delete. If we were doing this again today, we'd simply not ever install any filters by default. Leave it entirely upto the app/admin using libvirt to decide which filters are present. To that end we already split the filters off into a separate optional RPM, so that users can skip their installation. So if people want to modify these, we should really just be recommending that they not install this RPM in the first place Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 17 March 2015 at 19:04, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Mar 17, 2015 at 06:52:31PM +0000, Dimitri John Ledkov wrote:
libvirt runs correctly without any configuration files, as sensible defaults are used throughout. This commit introduces a layer for nwfilter configuration. This means that default filters are shipped in /usr/share/libvirt/nwfilter/ directory, which can be overridden by things in /etc/libvirt/nwfilter. This is similar to configuration splits as observed in udev, systemd, XDG Base Directory Specification and so on. This will make a distinction and make it obvious if any of the nwfilters are modified by the administrator.
I think this is really not something we want to do. Having multiple directories to store configs in is going to have more fallout than your patch shows. For a start this breaks the ability to delete these filters, since the deletion code assumes they're in a particular directory. Now if the same named filter is present in both locations the delete API does not know which to delete.
OK, I'll check that. So far I did simplistic testing and overwrote/remove filters by placing a filter without any rules but with the same name attribute in /etc and nwfilter-list / dumpxml still worked correctly. I will check the behaviour of define/undefine/edit as well.
If we were doing this again today, we'd simply not ever install any filters by default. Leave it entirely upto the app/admin using libvirt to decide which filters are present. To that end we already split the filters off into a separate optional RPM, so that users can skip their installation. So if people want to modify these, we should really just be recommending that they not install this RPM in the first place
Well... OpenStack software Nova component references the stock filters in their base configuration, e.g. no-mac-spoofing, no-ip-spoofing, no-arp-spoofing, allow-dhcp-server and so on. Thus these filters whilst example content, ended up being an API (removing them will break OpenStack Nova component). Hence the desire to move them away to /usr, as these shouldn't be modified, or at least that one can revert to stock versions of these. Would you be at all open to any other semantics with stock filters moved out of /etc? E.g. have filters loaded from /usr to not be modifiable / marked read-only from the API, or only allowed to be referenced using filterref? (thus making these filters essentially builtin/well-known aliases for typical filtering pieces). -- Regards, Dimitri. Open Source Technology Center Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
On Wed, Mar 18, 2015 at 10:09:53AM +0000, Dimitri John Ledkov wrote:
On 17 March 2015 at 19:04, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Mar 17, 2015 at 06:52:31PM +0000, Dimitri John Ledkov wrote:
libvirt runs correctly without any configuration files, as sensible defaults are used throughout. This commit introduces a layer for nwfilter configuration. This means that default filters are shipped in /usr/share/libvirt/nwfilter/ directory, which can be overridden by things in /etc/libvirt/nwfilter. This is similar to configuration splits as observed in udev, systemd, XDG Base Directory Specification and so on. This will make a distinction and make it obvious if any of the nwfilters are modified by the administrator.
I think this is really not something we want to do. Having multiple directories to store configs in is going to have more fallout than your patch shows. For a start this breaks the ability to delete these filters, since the deletion code assumes they're in a particular directory. Now if the same named filter is present in both locations the delete API does not know which to delete.
OK, I'll check that. So far I did simplistic testing and overwrote/remove filters by placing a filter without any rules but with the same name attribute in /etc and nwfilter-list / dumpxml still worked correctly. I will check the behaviour of define/undefine/edit as well.
If we were doing this again today, we'd simply not ever install any filters by default. Leave it entirely upto the app/admin using libvirt to decide which filters are present. To that end we already split the filters off into a separate optional RPM, so that users can skip their installation. So if people want to modify these, we should really just be recommending that they not install this RPM in the first place
Well... OpenStack software Nova component references the stock filters in their base configuration, e.g. no-mac-spoofing, no-ip-spoofing, no-arp-spoofing, allow-dhcp-server and so on. Thus these filters whilst example content, ended up being an API (removing them will break OpenStack Nova component). Hence the desire to move them away to /usr, as these shouldn't be modified, or at least that one can revert to stock versions of these.
I think i'd be better if Nova just defined its own set of rules for everything it needs rather than relying on what happens to be prsent, or not present, by default.
Would you be at all open to any other semantics with stock filters moved out of /etc? E.g. have filters loaded from /usr to not be modifiable / marked read-only from the API, or only allowed to be referenced using filterref? (thus making these filters essentially builtin/well-known aliases for typical filtering pieces).
No, that would be a semantic change in behaviour of the API which would potentially break applications Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Dimitri John Ledkov