[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]