Re: [Qemu-devel] [PATCH v3 2/2] qemu-bridge-helper: Use stateless configuration for bridge.conf

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

On Tue, May 26, 2015 at 01:57:41PM -0300, Eduardo Habkost wrote:
> On Tue, May 26, 2015 at 05:41:08PM +0100, Ikey Doherty wrote:
> > On 26/05/15 17:38, Eduardo Habkost wrote:
> > >On Tue, May 26, 2015 at 01:54:07PM +0100, Ikey Doherty wrote:
> > >>The goal of stateless, and thus this change, is to separate OS configuration
> > >>from system administrator configuration. With this change we will read the
> > >>default configuration data from /usr/share/defaults/qemu, in the absence of
> > >>an overriding site administrator configuration in /etc/qemu.
> > >>
> > >>A key advantage of this change is enabling a sane and immutable default OS
> > >>configuration, that is resiliant to upgrades. Ultimate power is still left
> > >>to the system administrator, with the ability to override the defaults if
> > >>required. Lastly, given that the sane defaults are always available, the
> > >>administrator may simply remove their site-config files to reset the
> > >>configuration to the "factory defaults" (i.e. OS configuration).
> > >>
> > >>Signed-off-by: Ikey Doherty <michael.i.doherty@intel.com>
> > >>---
> > >>  configure            |  2 ++
> > >>  qemu-bridge-helper.c | 15 +++++++++------
> > >>  2 files changed, 11 insertions(+), 6 deletions(-)
> > >>
> > >[...]
> > >>-#define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
> > >>+#define DEFAULT_ACL_FILE CONFIG_QEMU_DEFAULTDIR "/bridge.conf"
> > >>+#define SITE_ACL_FILE    CONFIG_QEMU_CONFDIR    "/bridge.conf"
> > >>
> > >>  enum {
> > >>      ACL_ALLOW = 0,
> > >>@@ -272,11 +273,13 @@ int main(int argc, char **argv)
> > >>
> > >>      /* parse default acl file */
> > >>      QSIMPLEQ_INIT(&acl_list);
> > >>-    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
> > >>-        fprintf(stderr, "failed to parse default acl file `%s'\n",
> > >>-                DEFAULT_ACL_FILE);
> > >>-        ret = EXIT_FAILURE;
> > >>-        goto cleanup;
> > >>+    if (parse_acl_file(SITE_ACL_FILE, &acl_list) == -1) {
> > >>+        if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
> > >>+            fprintf(stderr, "failed to parse default acl file `%s'\n",
> > >>+                    DEFAULT_ACL_FILE);
> > >>+            ret = EXIT_FAILURE;
> > >>+            goto cleanup;
> > >>+        }
> > >>      }
> > >
> > >This will make syntax errors on SITE_ACL_FILE cause partial loading of
> > >the rules on SITE_ACL_FILE, and trigger loading of DEFAULT_ACL_FILE,
> > >instead of aborting bridge-helper.
> > >
> > >Wouldn't it be better to fallback to DEFAULT_ACL_FILE if and only if
> > >SITE_ACL_FILE is missing?
> > >
> > 
> > I could stat the file and fallback to default, and only parse_acl_file
> > the one that exists. Better?
> 
> Or you could simply call parse_acl_file(DEFAULT_ACL_FILE) only if
> parse_acl_file(SITE_ACL_FILE) set errno=ENOENT.

Yes, please.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]