Re: [libvirt] [PATCH v2 0/7] Cleanup flags checking and fix setvcpus
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