Re: [libvirt] [PATCH 1/2] qemu: Refactor config parameter retrieval

> This patch adds macros to help retrieve configuration values from > qemu > driver's configuration. Some configuration options are grouped > together in the process. > --- > src/qemu/qemu_conf.c | 303 > +++++++++++++-------------------------------------- > 1 file changed, 73 insertions(+), 230 deletions(-) Always fun to see refactoring that reduces code size.

> +#define GET_VALUE_LONG(NAME, VAR) p = virConfGetValue(conf, NAME); > \ This feels like a long line; I'd format it slightly differently: #define GET_VALUE_LONG(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ ... > + CHECK_TYPE(NAME, VIR_CONF_LONG); > \ > + if (p) VAR = p->l; As long as you are refactoring, can you please split this line into two: if (p) \ VAR = p->l per our coding conventions?

> + > +#define GET_VALUE_STR(NAME, VAR) p = virConfGetValue(conf, NAME); > \ Again, this feels like a lot of indentation; starting the first line of the macro body after a continuation will get rid of a lot of this whitespace. > + /* increasing the value by 1 makes all the loops going through > + the bitmap (i = remotePortMin; i < remotePortMax; i++), work as > + expected. */ > + driver->remotePortMax += 1; Why ' += 1' instead of the shorter '++'?

> + GET_VALUE_LONG("max_queued", driver->max_queued); > + GET_VALUE_LONG("keepalive_interval", driver->keepAliveInterval); > + GET_VALUE_LONG("keepalive_count", driver->keepAliveCount); > + GET_VALUE_LONG("seccomp_sandbox", driver->seccompSandbox); > > virConfFree(conf); > return 0; Should we add #undef GET_VALUE_LONG #undef GET_VALUE_STRING at the end, since our macros are merely local helpers for this function?

ACK with nits addressed.

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list