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

[Peter Krempa]

On 11/29/12 20:04, Eric Blake wrote:
>> 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.

Indeed!

>
>> +#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?

Ah yeah ... I was doing the conversions in a kind of braindead way.

>
>> +
>> +#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 '++'?

It was pre-existing but I'm touching the line anyways so I'll change that.

>
>> +    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?

Definitely.

>
> ACK with nits addressed.

Pushed, thanks.
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>