[libvirt] [PATCH v2 0/7] Cleanup flags checking and fix setvcpus

[John Ferlan]

On 03/27/2015 06:01 AM, Pavel Hrdina wrote:
> The first four patches only cleanup the flags checking in our APIs by
> introducing new macros to check exclusive flags and requirements.
> 
> Patch 5/7 uses the new macros to do better flags checking for
> virDomainSetvcpusFlags API.
> 
> Patch 6/7 introduces macro to check virsh options requirements.
> 
> The last patch uses the requirement macro to cleanup virsh setvcpus code
> and fixes a bug with --maximum option.
> 
> Because only the last patch actually fixes a bug issue, I'm not sure whether
> this patch series should wait for next release cycle.
> 
> Luyao Huang (1):
>   tools: fix the wrong check when use virsh setvcpus --maximum
> 
> Pavel Hrdina (6):
>   internal: introduce macro helpers to reject exclusive flags
>   internal: introduce macro helpers to check flag requirements
>   use new macro helpers to check exclusive flags
>   use new macro helpers to check flag requirements
>   qemu: use new macros for setvcpus to check flags and cleanup the code
>   virsh: introduce new macros to help check flag requirements
> 
>  src/internal.h                     |  87 +++++++++++
>  src/libvirt-domain-snapshot.c      |  56 +++-----
>  src/libvirt-domain.c               | 286 +++++++++++--------------------------
>  src/qemu/qemu_driver.c             |  33 +----
>  src/storage/storage_backend_disk.c |  10 +-
>  src/storage/storage_backend_fs.c   |  11 +-
>  tools/virsh-domain.c               |  30 +---
>  tools/virsh.h                      |  52 +++++++
>  8 files changed, 256 insertions(+), 309 deletions(-)
> 

Since there's been too many changes since this was posted, these won't
'git am -3' for me, so it's a visual inspection...

Patches 1-4 -

 * In general - ACK - just be sure to heed Jeff's comment regarding use
of do { } while(0); within each #define.

 * On the plus side - less code, easier checks, and consistency with the
output. However...

 * This does change error message output from using strings like
'redefine', 'halt', running', 'paused', 'children', 'children_only', etc
to the #NAME-OF-FLAG output. Not that I find that a problem; however, I
have to wonder how that affects tests which search for certain strings
for failure case checking - eg. autotest/virt-test. This also has a side
effect on message i18n, but that's no different than any other message
outputting text in a message I suppose. We are reducing our messages.

 * You're also removing the __FUNCTION__ from the output for some errors
- I have no problem with that since it should be fairly obvious, but it
is different. Then again, the messages are

Patch 5 (ACK)
 * The error for GUEST && CONFIG changes - doesn't specifically call out
guest agent, but I would think someone supplying GUEST would know that
anyway, so it's a no big deal, except of course for the test script that
may look for a specific error message.

Patches 6-7 (ACK in general)

 * I agree with Jeff regarding the "do { } while(0);"

 * The commit message needs some massaging - it'd be nice to see the
output with the changes in place

 * What happens after these changes if someone does "virsh setvcpus
--current --maximum 10" to an inactive domain?

John