Re: [libvirt] [PATCH v2 0/7] Cleanup flags checking and fix setvcpus
On Thu, Apr 09, 2015 at 09:13:11AM -0400, John Ferlan wrote:
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.
Actually after this change it would be much easier for users of our APIs to
parse the error messages. They will have to deal with it, but it won't be
probably an issue for virt-tests, as they are using virsh and virsh already
checks a lot of those flags.
* 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
If you will check the virReportInvalidArg macro, you will see that there is
used __FUNCTION__. With this change all of the error messages will be logged
together with function name. And recently it was globally removed from the
error messages by commit c4db8c5e.
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
I'll come up with better commit message :).
* What happens after these changes if someone does "virsh setvcpus
--current --maximum 10" to an inactive domain?
It will fail with error, that --config is required. I was thinking about this
possibility to also enable use of --current together with --maximum, but it
doesn't make sense and it's actually pointless.
Thanks for the review. I'll wait with this series after release.
Pavel
John