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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list