[libvirt] [PATCH v2 0/7] Cleanup flags checking and fix setvcpus
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(-) -- 2.0.5
Inspired by commit 7e437ee7 that introduced similar macros for virsh commands so we don't have to repeat the same code all over. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/internal.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/internal.h b/src/internal.h index 4d473af..eb8d231 100644 --- a/src/internal.h +++ b/src/internal.h @@ -327,6 +327,50 @@ } \ } while (0) +/* Macros to help dealing with mutually exclusive flags. */ + +/** + * VIR_EXCLUSIVE_FLAGS_RET: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag to be checked. + * @RET: Return value. + * + * Reject mutually exclusive API flags. The checked flags are compared + * with flags variable. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_EXCLUSIVE_FLAGS_RET(FLAG1, FLAG2, RET) \ + if ((flags & FLAG1) && (flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + #FLAG1, #FLAG2); \ + return RET; \ + } + +/** + * VIR_EXCLUSIVE_FLAGS_GOTO: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag to be checked. + * @LABEL: Label to jump to. + * + * Reject mutually exclusive API flags. The checked flags are compared + * with flags variable. + * + * Returns nothing. Jumps to a label if unsupported flags were + * passed to it. + */ +# define VIR_EXCLUSIVE_FLAGS_GOTO(FLAG1, FLAG2, LABEL) \ + if ((flags & FLAG1) && (flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + #FLAG1, #FLAG2); \ + goto LABEL; \ + } + # define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \ -- 2.0.5
On Fri, Mar 27, 2015 at 11:01:22AM +0100, Pavel Hrdina wrote:
Inspired by commit 7e437ee7 that introduced similar macros for virsh commands so we don't have to repeat the same code all over.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/internal.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/src/internal.h b/src/internal.h index 4d473af..eb8d231 100644 --- a/src/internal.h +++ b/src/internal.h @@ -327,6 +327,50 @@ } \ } while (0)
+/* Macros to help dealing with mutually exclusive flags. */ + +/** + * VIR_EXCLUSIVE_FLAGS_RET: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag to be checked. + * @RET: Return value. + * + * Reject mutually exclusive API flags. The checked flags are compared + * with flags variable. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_EXCLUSIVE_FLAGS_RET(FLAG1, FLAG2, RET) \ + if ((flags & FLAG1) && (flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + #FLAG1, #FLAG2); \ + return RET; \ + }
Please encapsulate within a "do { ... } while(0)" statement. You've left an -if- statement hidden within the macro definition; the next -else- to come along will be matched to it: if (fred) VIR_EXCLUSIVE_FLAGS_GOTO(a,b,c) else // matches -if- *inside* macro // doesn't execute when you think it does
+ +/** + * VIR_EXCLUSIVE_FLAGS_GOTO: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag to be checked. + * @LABEL: Label to jump to. + * + * Reject mutually exclusive API flags. The checked flags are compared + * with flags variable. + * + * Returns nothing. Jumps to a label if unsupported flags were + * passed to it. + */ +# define VIR_EXCLUSIVE_FLAGS_GOTO(FLAG1, FLAG2, LABEL) \ + if ((flags & FLAG1) && (flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + #FLAG1, #FLAG2); \ + goto LABEL; \ + } +
Same feedback as above. Note the macro definition below does this properly. -Jeff
# define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \ -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Similar to VIR_EXLUSIVE_FLAGS, it will error out if flag requirement is not met. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/internal.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/internal.h b/src/internal.h index eb8d231..6363e58 100644 --- a/src/internal.h +++ b/src/internal.h @@ -371,6 +371,49 @@ goto LABEL; \ } +/* Macros to help dealing with flag requirements. */ + +/** + * VIR_REQUIRE_FLAG_RET: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag that is required by first flag. + * @RET: Return value. + * + * Check whether required flag is set. The checked flags are compared + * with flags variable. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_REQUIRE_FLAG_RET(FLAG1, FLAG2, RET) \ + if ((flags & FLAG1) && !(flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flag '%s' is required by flag '%s'"), \ + #FLAG2, #FLAG1); \ + return RET; \ + } + +/** + * VIR_REQUIRE_FLAG_GOTO: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag that is required by first flag. + * @LABEL: Label to jump to. + * + * Check whether required flag is set. The checked flags are compared + * with flags variable. + * + * Returns nothing. Jumps to a label if required flag is not set. + */ +# define VIR_REQUIRE_FLAG_GOTO(FLAG1, FLAG2, LABEL) \ + if ((flags & FLAG1) && !(flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flag '%s' is required by flag '%s'"), \ + #FLAG2, #FLAG1); \ + goto LABEL; \ + } + # define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \ -- 2.0.5
On Fri, Mar 27, 2015 at 11:01:23AM +0100, Pavel Hrdina wrote:
Similar to VIR_EXLUSIVE_FLAGS, it will error out if flag requirement is not met.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/internal.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/src/internal.h b/src/internal.h index eb8d231..6363e58 100644 --- a/src/internal.h +++ b/src/internal.h @@ -371,6 +371,49 @@ goto LABEL; \ }
+/* Macros to help dealing with flag requirements. */ + +/** + * VIR_REQUIRE_FLAG_RET: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag that is required by first flag. + * @RET: Return value. + * + * Check whether required flag is set. The checked flags are compared + * with flags variable. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_REQUIRE_FLAG_RET(FLAG1, FLAG2, RET) \ + if ((flags & FLAG1) && !(flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flag '%s' is required by flag '%s'"), \ + #FLAG2, #FLAG1); \ + return RET; \ + } +
I recommend encapsulating this within a "do { ... } while (1)" statement. The problem is that the macro hides/exposes an -if- statement to which an -else- keyword can be bound: if (something) VIR_REQUIRE_FLAG_RET(f1, f2, foo) else // matches with -if- inside VIR_REQUIRE_FLAG_RET // doesn't execute when you think it does Note that other macros (see definition of virCheckNonNullArgReturn below) follow this practice.
+/** + * VIR_REQUIRE_FLAG_GOTO: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag that is required by first flag. + * @LABEL: Label to jump to. + * + * Check whether required flag is set. The checked flags are compared + * with flags variable. + * + * Returns nothing. Jumps to a label if required flag is not set. + */ +# define VIR_REQUIRE_FLAG_GOTO(FLAG1, FLAG2, LABEL) \ + if ((flags & FLAG1) && !(flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flag '%s' is required by flag '%s'"), \ + #FLAG2, #FLAG1); \ + goto LABEL; \ + } +
Same feedback as above. -Jeff
# define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \ -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 27, 2015 at 11:13:07AM -0500, Jeff Nelson wrote:
On Fri, Mar 27, 2015 at 11:01:23AM +0100, Pavel Hrdina wrote:
Similar to VIR_EXLUSIVE_FLAGS, it will error out if flag requirement is not met.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/internal.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/src/internal.h b/src/internal.h index eb8d231..6363e58 100644 --- a/src/internal.h +++ b/src/internal.h @@ -371,6 +371,49 @@ goto LABEL; \ }
+/* Macros to help dealing with flag requirements. */ + +/** + * VIR_REQUIRE_FLAG_RET: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag that is required by first flag. + * @RET: Return value. + * + * Check whether required flag is set. The checked flags are compared + * with flags variable. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_REQUIRE_FLAG_RET(FLAG1, FLAG2, RET) \ + if ((flags & FLAG1) && !(flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flag '%s' is required by flag '%s'"), \ + #FLAG2, #FLAG1); \ + return RET; \ + } +
I recommend encapsulating this within a "do { ... } while (1)"
Oops. "while(0)", not "while (1)". -Jeff
statement. The problem is that the macro hides/exposes an -if- statement to which an -else- keyword can be bound:
if (something) VIR_REQUIRE_FLAG_RET(f1, f2, foo) else // matches with -if- inside VIR_REQUIRE_FLAG_RET // doesn't execute when you think it does
Note that other macros (see definition of virCheckNonNullArgReturn below) follow this practice.
+/** + * VIR_REQUIRE_FLAG_GOTO: + * + * @FLAG1: First flag to be checked. + * @FLAG2: Second flag that is required by first flag. + * @LABEL: Label to jump to. + * + * Check whether required flag is set. The checked flags are compared + * with flags variable. + * + * Returns nothing. Jumps to a label if required flag is not set. + */ +# define VIR_REQUIRE_FLAG_GOTO(FLAG1, FLAG2, LABEL) \ + if ((flags & FLAG1) && !(flags & FLAG2)) { \ + virReportInvalidArg(ctl, \ + _("Flag '%s' is required by flag '%s'"), \ + #FLAG2, #FLAG1); \ + goto LABEL; \ + } +
Same feedback as above.
-Jeff
# define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \ -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 45 ++---- src/libvirt-domain.c | 276 ++++++++++--------------------------- src/qemu/qemu_driver.c | 10 +- src/storage/storage_backend_disk.c | 10 +- src/storage/storage_backend_fs.c | 11 +- 5 files changed, 95 insertions(+), 257 deletions(-) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 9feb669..0d5c5e8 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -228,22 +228,13 @@ virDomainSnapshotCreateXML(virDomainPtr domain, __FUNCTION__); goto error; } - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && - (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { - virReportInvalidArg(flags, - _("'redefine' and 'no metadata' flags in %s are " - "mutually exclusive"), - __FUNCTION__); - goto error; - } - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && - (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { - virReportInvalidArg(flags, - _("'redefine' and 'halt' flags in %s are mutually " - "exclusive"), - __FUNCTION__); - goto error; - } + + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, + error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + VIR_DOMAIN_SNAPSHOT_CREATE_HALT, + error); if (conn->driver->domainSnapshotCreateXML) { virDomainSnapshotPtr ret; @@ -1082,14 +1073,9 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virCheckReadOnlyGoto(conn->flags, error); - if ((flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && - (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { - virReportInvalidArg(flags, - _("running and paused flags in %s are mutually " - "exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING, + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, + error); if (conn->driver->domainRevertToSnapshot) { int ret = conn->driver->domainRevertToSnapshot(snapshot, flags); @@ -1144,14 +1130,9 @@ virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, virCheckReadOnlyGoto(conn->flags, error); - if ((flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) && - (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { - virReportInvalidArg(flags, - _("children and children_only flags in %s are " - "mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, + error); if (conn->driver->domainSnapshotDelete) { int ret = conn->driver->domainSnapshotDelete(snapshot, flags); diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index f1608dc..af69d12 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -911,12 +911,9 @@ virDomainSaveFlags(virDomainPtr domain, const char *to, virCheckReadOnlyGoto(conn->flags, error); virCheckNonNullArgGoto(to, error); - if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virReportInvalidArg(flags, "%s", - _("running and paused flags are mutually " - "exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, + VIR_DOMAIN_SAVE_PAUSED, + error); if (conn->driver->domainSaveFlags) { int ret; @@ -1038,12 +1035,9 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, virCheckReadOnlyGoto(conn->flags, error); virCheckNonNullArgGoto(from, error); - if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virReportInvalidArg(flags, "%s", - _("running and paused flags are mutually " - "exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, + VIR_DOMAIN_SAVE_PAUSED, + error); if (conn->driver->domainRestoreFlags) { int ret; @@ -1179,12 +1173,9 @@ virDomainSaveImageDefineXML(virConnectPtr conn, const char *file, virCheckNonNullArgGoto(file, error); virCheckNonNullArgGoto(dxml, error); - if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virReportInvalidArg(flags, "%s", - _("running and paused flags are mutually " - "exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, + VIR_DOMAIN_SAVE_PAUSED, + error); if (conn->driver->domainSaveImageDefineXML) { int ret; @@ -1257,23 +1248,9 @@ virDomainCoreDump(virDomainPtr domain, const char *to, unsigned int flags) virCheckReadOnlyGoto(conn->flags, error); virCheckNonNullArgGoto(to, error); - if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_LIVE)) { - virReportInvalidArg(flags, "%s", - _("crash and live flags are mutually exclusive")); - goto error; - } - - if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_RESET)) { - virReportInvalidArg(flags, "%s", - _("crash and reset flags are mutually exclusive")); - goto error; - } - - if ((flags & VIR_DUMP_LIVE) && (flags & VIR_DUMP_RESET)) { - virReportInvalidArg(flags, "%s", - _("live and reset flags are mutually exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DUMP_CRASH, VIR_DUMP_LIVE, error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DUMP_CRASH, VIR_DUMP_RESET, error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DUMP_LIVE, VIR_DUMP_RESET, error); if (conn->driver->domainCoreDump) { int ret; @@ -1355,23 +1332,9 @@ virDomainCoreDumpWithFormat(virDomainPtr domain, const char *to, goto error; } - if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_LIVE)) { - virReportInvalidArg(flags, "%s", - _("crash and live flags are mutually exclusive")); - goto error; - } - - if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_RESET)) { - virReportInvalidArg(flags, "%s", - _("crash and reset flags are mutually exclusive")); - goto error; - } - - if ((flags & VIR_DUMP_LIVE) && (flags & VIR_DUMP_RESET)) { - virReportInvalidArg(flags, "%s", - _("live and reset flags are mutually exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DUMP_CRASH, VIR_DUMP_LIVE, error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DUMP_CRASH, VIR_DUMP_RESET, error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DUMP_LIVE, VIR_DUMP_RESET, error); if (conn->driver->domainCoreDumpWithFormat) { int ret; @@ -2176,15 +2139,10 @@ virDomainGetMemoryParameters(virDomainPtr domain, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); + conn = domain->conn; if (conn->driver->domainGetMemoryParameters) { @@ -2420,15 +2378,10 @@ virDomainGetBlkioParameters(virDomainPtr domain, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); + conn = domain->conn; if (conn->driver->domainGetBlkioParameters) { @@ -3578,14 +3531,9 @@ virDomainMigrate(virDomainPtr domain, virCheckConnectGoto(dconn, error); virCheckReadOnlyGoto(dconn->flags, error); - if (flags & VIR_MIGRATE_NON_SHARED_DISK && - flags & VIR_MIGRATE_NON_SHARED_INC) { - virReportInvalidArg(flags, - _("flags 'shared disk' and 'shared incremental' " - "in %s are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + error); if (flags & VIR_MIGRATE_OFFLINE) { if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, @@ -3807,14 +3755,9 @@ virDomainMigrate2(virDomainPtr domain, virCheckConnectGoto(dconn, error); virCheckReadOnlyGoto(dconn->flags, error); - if (flags & VIR_MIGRATE_NON_SHARED_DISK && - flags & VIR_MIGRATE_NON_SHARED_INC) { - virReportInvalidArg(flags, - _("flags 'shared disk' and 'shared incremental' " - "in %s are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + error); if (flags & VIR_MIGRATE_OFFLINE) { if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, @@ -3987,14 +3930,10 @@ virDomainMigrate3(virDomainPtr domain, virCheckConnectGoto(dconn, error); virCheckReadOnlyGoto(dconn->flags, error); - if (flags & VIR_MIGRATE_NON_SHARED_DISK && - flags & VIR_MIGRATE_NON_SHARED_INC) { - virReportInvalidArg(flags, - _("flags 'shared disk' and 'shared incremental' " - "in %s are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + error); + if (flags & VIR_MIGRATE_PEER2PEER) { virReportInvalidArg(flags, "%s", _("use virDomainMigrateToURI3 for peer-to-peer " @@ -4210,14 +4149,9 @@ virDomainMigrateToURI(virDomainPtr domain, virCheckNonNullArgGoto(duri, error); - if (flags & VIR_MIGRATE_NON_SHARED_DISK && - flags & VIR_MIGRATE_NON_SHARED_INC) { - virReportInvalidArg(flags, - _("flags 'shared disk' and 'shared incremental' " - "in %s are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + error); if (flags & VIR_MIGRATE_OFFLINE && !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, @@ -4370,14 +4304,9 @@ virDomainMigrateToURI2(virDomainPtr domain, virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - if (flags & VIR_MIGRATE_NON_SHARED_DISK && - flags & VIR_MIGRATE_NON_SHARED_INC) { - virReportInvalidArg(flags, - _("flags 'shared disk' and 'shared incremental' " - "in %s are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + error); if (flags & VIR_MIGRATE_PEER2PEER) { if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, @@ -4479,14 +4408,9 @@ virDomainMigrateToURI3(virDomainPtr domain, virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - if (flags & VIR_MIGRATE_NON_SHARED_DISK && - flags & VIR_MIGRATE_NON_SHARED_INC) { - virReportInvalidArg(flags, - _("flags 'shared disk' and 'shared incremental' " - "in %s are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_NON_SHARED_DISK, + VIR_MIGRATE_NON_SHARED_INC, + error); compat = virTypedParamsCheck(params, nparams, compatParams, ARRAY_CARDINALITY(compatParams)); @@ -5526,15 +5450,10 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); + conn = domain->conn; if (conn->driver->domainGetSchedulerParametersFlags) { @@ -6295,14 +6214,7 @@ virDomainMemoryPeek(virDomainPtr dom, * because of incompatible licensing. */ - /* Exactly one of these two flags must be set. */ - if (!(flags & VIR_MEMORY_VIRTUAL) == !(flags & VIR_MEMORY_PHYSICAL)) { - virReportInvalidArg(flags, - _("flags in %s must include VIR_MEMORY_VIRTUAL or " - "VIR_MEMORY_PHYSICAL"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MEMORY_VIRTUAL, VIR_MEMORY_PHYSICAL, error); /* Allow size == 0 as an access test. */ if (size > 0) @@ -7340,14 +7252,9 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); - if (flags & VIR_DOMAIN_VCPU_GUEST && - flags & VIR_DOMAIN_VCPU_MAXIMUM) { - virReportInvalidArg(flags, - _("flags 'VIR_DOMAIN_VCPU_MAXIMUM' and " - "'VIR_DOMAIN_VCPU_GUEST' in '%s' are mutually " - "exclusive"), __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST, + VIR_DOMAIN_VCPU_MAXIMUM, + error); virCheckNonZeroArgGoto(nvcpus, error); @@ -7416,15 +7323,9 @@ virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) if (flags & VIR_DOMAIN_VCPU_GUEST) virCheckReadOnlyGoto(conn->flags, error); - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); if (conn->driver->domainGetVcpusFlags) { int ret; @@ -7622,15 +7523,9 @@ virDomainGetVcpuPinInfo(virDomainPtr domain, int ncpumaps, goto error; } - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); if (conn->driver->domainGetVcpuPinInfo) { int ret; @@ -7752,15 +7647,10 @@ virDomainGetEmulatorPinInfo(virDomainPtr domain, unsigned char *cpumap, virCheckNonNullArgGoto(cpumap, error); virCheckPositiveArgGoto(maplen, error); - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); + conn = domain->conn; if (conn->driver->domainGetEmulatorPinInfo) { @@ -7922,15 +7812,9 @@ virDomainGetIOThreadInfo(virDomainPtr dom, virCheckNonNullArgGoto(info, error); *info = NULL; - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); if (dom->conn->driver->domainGetIOThreadInfo) { int ret; @@ -8257,14 +8141,9 @@ virDomainGetMetadata(virDomainPtr domain, virCheckDomainReturn(domain, NULL); - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); switch (type) { case VIR_DOMAIN_METADATA_TITLE: @@ -9309,13 +9188,9 @@ virDomainManagedSave(virDomainPtr dom, unsigned int flags) virCheckReadOnlyGoto(conn->flags, error); - if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virReportInvalidArg(flags, - _("running and paused flags in %s are mutually " - "exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING, + VIR_DOMAIN_SAVE_PAUSED, + error); if (conn->driver->domainManagedSave) { int ret; @@ -10486,15 +10361,10 @@ virDomainGetBlockIoTune(virDomainPtr dom, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && - (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - virReportInvalidArg(flags, - _("flags 'affect live' and 'affect config' in %s " - "are mutually exclusive"), - __FUNCTION__); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error); + conn = dom->conn; if (conn->driver->domainGetBlockIoTune) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f07e4fb..f7d77e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2015,13 +2015,9 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN | VIR_DOMAIN_REBOOT_GUEST_AGENT, -1); - /* At most one of these two flags should be set. */ - if ((flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN) && - (flags & VIR_DOMAIN_REBOOT_GUEST_AGENT)) { - virReportInvalidArg(flags, "%s", - _("flags for acpi power button and guest agent are mutually exclusive")); - return -1; - } + VIR_EXCLUSIVE_FLAGS_RET(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN, + VIR_DOMAIN_REBOOT_GUEST_AGENT, + -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index ba928d8..3689e8e 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -441,13 +441,9 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); - if (flags == (VIR_STORAGE_POOL_BUILD_OVERWRITE | - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Overwrite and no overwrite flags" - " are mutually exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE, + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, + error); if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { ok_to_mklabel = true; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 35385db..9ce97d6 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -774,14 +774,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); - if (flags == (VIR_STORAGE_POOL_BUILD_OVERWRITE | - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE)) { - - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Overwrite and no overwrite flags" - " are mutually exclusive")); - goto error; - } + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE, + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, + error); if (VIR_STRDUP(parent, pool->def->target.path) < 0) goto error; -- 2.0.5
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 11 +++-------- src/qemu/qemu_driver.c | 9 +++------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 0d5c5e8..9d43f54 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -220,14 +220,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, virCheckNonNullArgGoto(xmlDesc, error); virCheckReadOnlyGoto(conn->flags, error); - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) && - !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { - virReportInvalidArg(flags, - _("use of 'current' flag in %s requires " - "'redefine' flag"), - __FUNCTION__); - goto error; - } + VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + error); VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7d77e7..a5ee99d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14541,12 +14541,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) && - !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("quiesce requires disk-only")); - return NULL; - } + VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, + NULL); if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) -- 2.0.5
Now that we have macros for exclusive flags and flag requirements we can use them to cleanup the code for setvcpus and error out for all wrong flag combination. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain.c | 12 +++++++++++- src/qemu/qemu_driver.c | 14 -------------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index af69d12..a4ae327 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7252,8 +7252,18 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, virCheckDomainReturn(domain, -1); virCheckReadOnlyGoto(domain->conn->flags, error); + VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_VCPU_MAXIMUM, + VIR_DOMAIN_AFFECT_CONFIG, + error); + + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT, + VIR_DOMAIN_AFFECT_LIVE, + error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT, + VIR_DOMAIN_AFFECT_CONFIG, + error); VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST, - VIR_DOMAIN_VCPU_MAXIMUM, + VIR_DOMAIN_AFFECT_CONFIG, error); virCheckNonZeroArgGoto(nvcpus, error); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5ee99d..aa58aa2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4902,13 +4902,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - /* MAXIMUM cannot be mixed with LIVE. */ - if ((flags & VIR_DOMAIN_VCPU_MAXIMUM) && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("cannot adjust maximum vcpus on running domain")); - goto endjob; - } - if (flags & VIR_DOMAIN_AFFECT_LIVE) maxvcpus = vm->def->maxvcpus; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -4924,13 +4917,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, } if (flags & VIR_DOMAIN_VCPU_GUEST) { - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("setting vcpus via guest agent isn't supported " - "on offline domain")); - goto endjob; - } - if (!qemuDomainAgentAvailable(vm, true)) goto endjob; -- 2.0.5
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tools/virsh.h b/tools/virsh.h index df2ea7f..fde20ef 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -474,4 +474,56 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, # define VSH_EXCLUSIVE_OPTIONS_VAR(VARNAME1, VARNAME2) \ VSH_EXCLUSIVE_OPTIONS_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2) +/* Macros to help dealing with required options. */ + +/* VSH_REQUIRE_OPTION_EXPR: + * + * @NAME1: String containing the name of the option. + * @EXPR1: Expression to validate the variable (boolean variable). + * @NAME2: String containing the name of required option. + * @EXPR2: Expression to validate the variable (boolean variable). + * + * Check if required command options in virsh was set. Use the + * provided expression to check the variables. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION_EXPR(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && !(EXPR2)) { \ + vshError(ctl, _("Option --%s is required by option --%s"), \ + NAME2, NAME1); \ + return false; \ + } + +/* VSH_REQUIRE_OPTION: + * + * @NAME1: String containing the name of the option. + * @NAME2: String containing the name of required option. + * + * Check if required command options in virsh was set. Use the + * vshCommandOptBool call to request them. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION(NAME1, NAME2) \ + VSH_REQUIRE_OPTION_EXPR(NAME1, vshCommandOptBool(cmd, NAME1), \ + NAME2, vshCommandOptBool(cmd, NAME2)) + +/* VSH_REQUIRE_OPTION_VAR: + * + * @VARNAME1: Boolean variable containing the value of the option of same name. + * @VARNAME2: Boolean variable containing the value of required option of + * same name. + * + * Check if required command options in virsh was set. Check in variables + * that contain the value and have same name as the option. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION_VAR(VARNAME1, VARNAME2) \ + VSH_REQUIRE_OPTION_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2) + #endif /* VIRSH_H */ -- 2.0.5
On Fri, Mar 27, 2015 at 11:01:27AM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
diff --git a/tools/virsh.h b/tools/virsh.h index df2ea7f..fde20ef 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -474,4 +474,56 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, # define VSH_EXCLUSIVE_OPTIONS_VAR(VARNAME1, VARNAME2) \ VSH_EXCLUSIVE_OPTIONS_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2)
+/* Macros to help dealing with required options. */ + +/* VSH_REQUIRE_OPTION_EXPR: + * + * @NAME1: String containing the name of the option. + * @EXPR1: Expression to validate the variable (boolean variable). + * @NAME2: String containing the name of required option. + * @EXPR2: Expression to validate the variable (boolean variable). + * + * Check if required command options in virsh was set. Use the + * provided expression to check the variables. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION_EXPR(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && !(EXPR2)) { \ + vshError(ctl, _("Option --%s is required by option --%s"), \ + NAME2, NAME1); \ + return false; \ + }
It would be better to protect this within a "do { ... } while (0)" statement. -Jeff
+ +/* VSH_REQUIRE_OPTION: + * + * @NAME1: String containing the name of the option. + * @NAME2: String containing the name of required option. + * + * Check if required command options in virsh was set. Use the + * vshCommandOptBool call to request them. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION(NAME1, NAME2) \ + VSH_REQUIRE_OPTION_EXPR(NAME1, vshCommandOptBool(cmd, NAME1), \ + NAME2, vshCommandOptBool(cmd, NAME2)) + +/* VSH_REQUIRE_OPTION_VAR: + * + * @VARNAME1: Boolean variable containing the value of the option of same name. + * @VARNAME2: Boolean variable containing the value of required option of + * same name. + * + * Check if required command options in virsh was set. Check in variables + * that contain the value and have same name as the option. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION_VAR(VARNAME1, VARNAME2) \ + VSH_REQUIRE_OPTION_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2) + #endif /* VIRSH_H */ -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 27, 2015 at 11:19:35AM -0500, Jeff Nelson wrote:
On Fri, Mar 27, 2015 at 11:01:27AM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
diff --git a/tools/virsh.h b/tools/virsh.h index df2ea7f..fde20ef 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -474,4 +474,56 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, # define VSH_EXCLUSIVE_OPTIONS_VAR(VARNAME1, VARNAME2) \ VSH_EXCLUSIVE_OPTIONS_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2)
+/* Macros to help dealing with required options. */ + +/* VSH_REQUIRE_OPTION_EXPR: + * + * @NAME1: String containing the name of the option. + * @EXPR1: Expression to validate the variable (boolean variable). + * @NAME2: String containing the name of required option. + * @EXPR2: Expression to validate the variable (boolean variable). + * + * Check if required command options in virsh was set. Use the + * provided expression to check the variables. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION_EXPR(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && !(EXPR2)) { \ + vshError(ctl, _("Option --%s is required by option --%s"), \ + NAME2, NAME1); \ + return false; \ + }
It would be better to protect this within a "do { ... } while (0)" statement.
Thanks for the review, I'll update all the macros. Pavel
-Jeff
+ +/* VSH_REQUIRE_OPTION: + * + * @NAME1: String containing the name of the option. + * @NAME2: String containing the name of required option. + * + * Check if required command options in virsh was set. Use the + * vshCommandOptBool call to request them. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION(NAME1, NAME2) \ + VSH_REQUIRE_OPTION_EXPR(NAME1, vshCommandOptBool(cmd, NAME1), \ + NAME2, vshCommandOptBool(cmd, NAME2)) + +/* VSH_REQUIRE_OPTION_VAR: + * + * @VARNAME1: Boolean variable containing the value of the option of same name. + * @VARNAME2: Boolean variable containing the value of required option of + * same name. + * + * Check if required command options in virsh was set. Check in variables + * that contain the value and have same name as the option. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VSH_REQUIRE_OPTION_VAR(VARNAME1, VARNAME2) \ + VSH_REQUIRE_OPTION_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2) + #endif /* VIRSH_H */ -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
From: Luyao Huang <lhuang@redhat.com> We will ignore --maximum option when only use setvcpus with this option, like this (this error is another issue): # virsh setvcpus test3 --maximum 10 error: Failed to create controller cpu for group: No such file or directory this is because we do not set it in flags before we check if there is a flags set. Refactor these code and fix the logic. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033 Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh-domain.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b39f4b6..1938f56 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6736,15 +6736,16 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(guest, config); + VSH_REQUIRE_OPTION_VAR(maximum, config); + if (config) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; if (guest) flags |= VIR_DOMAIN_VCPU_GUEST; - /* none of the options were specified */ - if (!current && flags == 0) - flags = -1; + if (maximum) + flags |= VIR_DOMAIN_VCPU_MAXIMUM; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -6754,30 +6755,11 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (flags == -1) { + /* none of the options were specified */ + if (!current && flags == 0) { if (virDomainSetVcpus(dom, count) != 0) goto cleanup; } else { - /* If the --maximum flag was given, we need to ensure only the - --config flag is in effect as well */ - if (maximum) { - vshDebug(ctl, VSH_ERR_DEBUG, "--maximum flag was given\n"); - - flags |= VIR_DOMAIN_VCPU_MAXIMUM; - - /* If neither the --config nor --live flags were given, OR - if just the --live flag was given, we need to error out - warning the user that the --maximum flag can only be used - with the --config flag */ - if (live || !config) { - - /* Warn the user about the invalid flag combination */ - vshError(ctl, _("--maximum must be used with --config only")); - goto cleanup; - } - } - - /* Apply the virtual cpu changes */ if (virDomainSetVcpusFlags(dom, count, flags) < 0) goto cleanup; } -- 2.0.5
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
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
participants (3)
-
Jeff Nelson -
John Ferlan -
Pavel Hrdina