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