[libvirt] [PATCH 0/6] Some cleanups, improvements and bug fixes of flags usage
Two bugs has been filed about wrong usage of api flags and virsh commands. After investigation of the core issue I've decide to introduce same helper macros as we have in virsh code to check mutually exclusive flags. This patch series also includes the bug fixes and some other cleanups in setvcpus code. Luyao Huang (2): qemu: check if domain is really active when do setvcpus with --live tools: fix the wrong check when use virsh setvcpus --maximum Pavel Hrdina (4): internal: introduce macro helpers to reject exclusive flags use new macro helpers to check exclusive flags qemu: cleanup setvcpus qemu: fix set vcpus on host without NUMA src/internal.h | 80 +++++++++++ src/libvirt-domain-snapshot.c | 45 ++---- src/libvirt-domain.c | 288 +++++++++++-------------------------- src/qemu/qemu_driver.c | 42 +++--- src/storage/storage_backend_disk.c | 10 +- src/storage/storage_backend_fs.c | 11 +- tools/virsh-domain.c | 30 +--- 7 files changed, 208 insertions(+), 298 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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/internal.h b/src/internal.h index 4d473af..5885f03 100644 --- a/src/internal.h +++ b/src/internal.h @@ -327,6 +327,86 @@ } \ } while (0) +/* Macros to help dealing with mutually exclusive flags. */ + +/** + * VIR_EXCLUSIVE_FLAGS_EXPR_RET: + * + * @NAME1: String containing the name of the flag. + * @EXPR1: Expression to validate the flag. + * @NAME2: String containing the name of the flag. + * @EXPR2: Expression to validate the flag. + * + * Reject mutually exclusive API flags. Use the provided expression + * to check the flags. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_EXCLUSIVE_FLAGS_EXPR_RET(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && (EXPR2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + NAME1, NAME2); \ + return -1; \ + } + +/** + * VIR_EXCLUSIVE_FLAGS_VAR_RET: + * + * @FLAG1: First flag to be checked + * @FLAG2: Second flag to be checked + * + * 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) \ + VIR_EXCLUSIVE_FLAGS_EXPR_RET(#FLAG1, flags & FLAG1, #FLAG2, flags & FLAG2) + +/** + * VIR_EXCLUSIVE_FLAGS_EXPR_GOTO: + * + * @NAME1: String containing the name of the flag. + * @EXPR1: Expression to validate the flag. + * @NAME2: String containing the name of the flag. + * @EXPR2: Expression to validate the flag. + * @LABEL: label to jump to. + * + * Reject mutually exclusive API flags. Use the provided expression + * to check the flag. + * + * Returns nothing. Jumps to a label if unsupported flags were + * passed to it. + */ +# define VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(NAME1, EXPR1, NAME2, EXPR2, LABEL) \ + if ((EXPR1) && (EXPR2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + NAME1, NAME2); \ + goto LABEL; \ + } + +/** + * VIR_EXCLUSIVE_FLAGS_VAR_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) \ + VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(#FLAG1, flags & FLAG1, \ + #FLAG2, flags & FLAG2, LABEL) + + # define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \ -- 2.0.5
We should call virDomainLiveConfigHelperMethod ASAP because this function transfers VIR_DOMAIN_AFFECT_CURRENT to VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. All other additional checks for those two flags should consider that the user give us VIR_DOMAIN_AFFECT_CURRENT. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eb86d68..575d3b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4881,6 +4881,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4905,10 +4909,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) - 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", -- 2.0.5
On Fri, Mar 20, 2015 at 15:58:56 +0100, Pavel Hrdina wrote:
We should call virDomainLiveConfigHelperMethod ASAP because this function transfers VIR_DOMAIN_AFFECT_CURRENT to VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. All other additional checks for those two flags should consider that the user give us VIR_DOMAIN_AFFECT_CURRENT.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK, although this should be 0.5/6 or 1.5/6. Basically just push this first. Peter
On Tue, Mar 24, 2015 at 05:13:07PM +0100, Peter Krempa wrote:
On Fri, Mar 20, 2015 at 15:58:56 +0100, Pavel Hrdina wrote:
We should call virDomainLiveConfigHelperMethod ASAP because this function transfers VIR_DOMAIN_AFFECT_CURRENT to VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. All other additional checks for those two flags should consider that the user give us VIR_DOMAIN_AFFECT_CURRENT.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK, although this should be 0.5/6 or 1.5/6. Basically just push this first.
Peter
Thanks for review, I'll push it shortly. Pavel
On Fri, Mar 20, 2015 at 15:38:59 +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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
diff --git a/src/internal.h b/src/internal.h index 4d473af..5885f03 100644 --- a/src/internal.h +++ b/src/internal.h @@ -327,6 +327,86 @@ } \ } while (0)
+/* Macros to help dealing with mutually exclusive flags. */ + +/** + * VIR_EXCLUSIVE_FLAGS_EXPR_RET: + * + * @NAME1: String containing the name of the flag. + * @EXPR1: Expression to validate the flag. + * @NAME2: String containing the name of the flag. + * @EXPR2: Expression to validate the flag. + * + * Reject mutually exclusive API flags. Use the provided expression + * to check the flags. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_EXCLUSIVE_FLAGS_EXPR_RET(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && (EXPR2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + NAME1, NAME2); \ + return -1; \ + } +
While in virsh the above variant makes sense, because in some cases the variables have different names than the corresponding flags, in case of this code having it doesn't make sense.
+/** + * VIR_EXCLUSIVE_FLAGS_VAR_RET: + * + * @FLAG1: First flag to be checked + * @FLAG2: Second flag to be checked
A third argument @RET should be added here so that this can be used in places that also return NULL or any other possible variable. In virsh the assumption is that the flags are checked at first and thus returning false makes sense, as every virsh command does that.
+ * + * 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) \ + VIR_EXCLUSIVE_FLAGS_EXPR_RET(#FLAG1, flags & FLAG1, #FLAG2, flags & FLAG2) + +/** + * VIR_EXCLUSIVE_FLAGS_EXPR_GOTO: + * + * @NAME1: String containing the name of the flag. + * @EXPR1: Expression to validate the flag. + * @NAME2: String containing the name of the flag. + * @EXPR2: Expression to validate the flag. + * @LABEL: label to jump to. + * + * Reject mutually exclusive API flags. Use the provided expression + * to check the flag. + * + * Returns nothing. Jumps to a label if unsupported flags were + * passed to it. + */ +# define VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(NAME1, EXPR1, NAME2, EXPR2, LABEL) \ + if ((EXPR1) && (EXPR2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + NAME1, NAME2); \ + goto LABEL; \ + } +
Again, this function does not make sense in the library code.
+/** + * VIR_EXCLUSIVE_FLAGS_VAR_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) \ + VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(#FLAG1, flags & FLAG1, \ + #FLAG2, flags & FLAG2, LABEL) + + # define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \
While this is okay. Peter
On Tue, Mar 24, 2015 at 04:40:17PM +0100, Peter Krempa wrote:
On Fri, Mar 20, 2015 at 15:38:59 +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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
diff --git a/src/internal.h b/src/internal.h index 4d473af..5885f03 100644 --- a/src/internal.h +++ b/src/internal.h @@ -327,6 +327,86 @@ } \ } while (0)
+/* Macros to help dealing with mutually exclusive flags. */ + +/** + * VIR_EXCLUSIVE_FLAGS_EXPR_RET: + * + * @NAME1: String containing the name of the flag. + * @EXPR1: Expression to validate the flag. + * @NAME2: String containing the name of the flag. + * @EXPR2: Expression to validate the flag. + * + * Reject mutually exclusive API flags. Use the provided expression + * to check the flags. + * + * This helper does an early return and therefore it has to be called + * before anything that would require cleanup. + */ +# define VIR_EXCLUSIVE_FLAGS_EXPR_RET(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && (EXPR2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + NAME1, NAME2); \ + return -1; \ + } +
While in virsh the above variant makes sense, because in some cases the variables have different names than the corresponding flags, in case of this code having it doesn't make sense.
You're right. I've only used the other macros.
+/** + * VIR_EXCLUSIVE_FLAGS_VAR_RET: + * + * @FLAG1: First flag to be checked + * @FLAG2: Second flag to be checked
A third argument @RET should be added here so that this can be used in places that also return NULL or any other possible variable.
In virsh the assumption is that the flags are checked at first and thus returning false makes sense, as every virsh command does that.
I'll add the @RET argument.
+ * + * 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) \ + VIR_EXCLUSIVE_FLAGS_EXPR_RET(#FLAG1, flags & FLAG1, #FLAG2, flags & FLAG2) + +/** + * VIR_EXCLUSIVE_FLAGS_EXPR_GOTO: + * + * @NAME1: String containing the name of the flag. + * @EXPR1: Expression to validate the flag. + * @NAME2: String containing the name of the flag. + * @EXPR2: Expression to validate the flag. + * @LABEL: label to jump to. + * + * Reject mutually exclusive API flags. Use the provided expression + * to check the flag. + * + * Returns nothing. Jumps to a label if unsupported flags were + * passed to it. + */ +# define VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(NAME1, EXPR1, NAME2, EXPR2, LABEL) \ + if ((EXPR1) && (EXPR2)) { \ + virReportInvalidArg(ctl, \ + _("Flags '%s' and '%s' are mutually exclusive"),\ + NAME1, NAME2); \ + goto LABEL; \ + } +
Again, this function does not make sense in the library code.
+/** + * VIR_EXCLUSIVE_FLAGS_VAR_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) \ + VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(#FLAG1, flags & FLAG1, \ + #FLAG2, flags & FLAG2, LABEL) + + # define virCheckNonNullArgReturn(argname, retval) \ do { \ if (argname == NULL) { \
While this is okay.
Peter
Thanks for review, I'll send v2. Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 45 ++---- src/libvirt-domain.c | 288 +++++++++++-------------------------- src/qemu/qemu_driver.c | 9 +- src/storage/storage_backend_disk.c | 10 +- src/storage/storage_backend_fs.c | 11 +- 5 files changed, 106 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 0bd9274..5482c70 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,21 @@ 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_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_AFFECT_CONFIG, + error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST, + VIR_DOMAIN_VCPU_MAXIMUM, + error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CONFIG, + VIR_DOMAIN_VCPU_MAXIMUM, + error); virCheckNonZeroArgGoto(nvcpus, error); @@ -7416,15 +7335,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 +7535,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 +7659,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 +7824,9 @@ virDomainGetIOThreadsInfo(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->domainGetIOThreadsInfo) { int ret; @@ -8257,14 +8153,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 +9200,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 +10373,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 6d9217b..cc6656d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2015,13 +2015,8 @@ 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); 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
On Fri, Mar 20, 2015 at 15:39:00 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 45 ++---- src/libvirt-domain.c | 288 +++++++++++-------------------------- src/qemu/qemu_driver.c | 9 +- src/storage/storage_backend_disk.c | 10 +- src/storage/storage_backend_fs.c | 11 +- 5 files changed, 106 insertions(+), 257 deletions(-)
I don't object to this change, but as this changes a lot of error messages I'd like to have a second opinion on this. Peter
On Fri, Mar 20, 2015 at 15:39:00 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 45 ++---- src/libvirt-domain.c | 288 +++++++++++-------------------------- src/qemu/qemu_driver.c | 9 +- src/storage/storage_backend_disk.c | 10 +- src/storage/storage_backend_fs.c | 11 +- 5 files changed, 106 insertions(+), 257 deletions(-)
@@ -7340,14 +7252,21 @@ 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_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_AFFECT_CONFIG, + error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST, + VIR_DOMAIN_VCPU_MAXIMUM, + error); + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CONFIG, + VIR_DOMAIN_VCPU_MAXIMUM, + error);
By the way, this check here is not enough to check that MAXIMUM is not actually used with _AFFECT_LIVE. If you use VIR_DOMAIN_AFFECT_CURRENT and the guest is online, this check is bypassed as the state of the domain is not known at this point. This unfortunately needs to be checked after the virDomainLiveConfigHelperMethod in the actual code.
virCheckNonZeroArgGoto(nvcpus, error);
Peter
Remove unnecessary maximum variable and also exclusive flags check as they are now tested by upper layer for all drivers. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cc6656d..4942712 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4841,7 +4841,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef; int ret = -1; - bool maximum; unsigned int maxvcpus = 0; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; @@ -4897,17 +4896,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - maximum = (flags & VIR_DOMAIN_VCPU_MAXIMUM) != 0; - flags &= ~VIR_DOMAIN_VCPU_MAXIMUM; - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, &persistentDef) < 0) goto endjob; /* MAXIMUM cannot be mixed with LIVE. */ - if (maximum && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + if ((flags & VIR_DOMAIN_VCPU_MAXIMUM) && (flags & VIR_DOMAIN_AFFECT_LIVE)) { virReportError(VIR_ERR_INVALID_ARG, "%s", - _("cannot adjust maximum on running domain")); + _("cannot adjust maximum vcpus on running domain")); goto endjob; } @@ -4917,7 +4913,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (!maxvcpus || maxvcpus > persistentDef->maxvcpus) maxvcpus = persistentDef->maxvcpus; } - if (!maximum && nvcpus > maxvcpus) { + if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && nvcpus > maxvcpus) { virReportError(VIR_ERR_INVALID_ARG, _("requested vcpus is greater than max allowable" " vcpus for the domain: %d > %d"), @@ -4928,8 +4924,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (flags & VIR_DOMAIN_VCPU_GUEST) { if (flags & VIR_DOMAIN_AFFECT_CONFIG) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("changing of maximum vCPU count isn't supported " - "via guest agent")); + _("setting vcpus via guest agent isn't supported " + "on offline domain")); goto endjob; } @@ -4992,7 +4988,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, i); } - if (maximum) { + if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { persistentDef->maxvcpus = nvcpus; if (nvcpus < persistentDef->vcpus) persistentDef->vcpus = nvcpus; -- 2.0.5
On Fri, Mar 20, 2015 at 15:39:01 +0100, Pavel Hrdina wrote:
Remove unnecessary maximum variable and also exclusive flags check as they are now tested by upper layer for all drivers.
The part about the exclusive flag check is not true.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
ACK with commit message fixed. Peter
On Tue, Mar 24, 2015 at 05:01:09PM +0100, Peter Krempa wrote:
On Fri, Mar 20, 2015 at 15:39:01 +0100, Pavel Hrdina wrote:
Remove unnecessary maximum variable and also exclusive flags check as they are now tested by upper layer for all drivers.
The part about the exclusive flag check is not true.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
ACK with commit message fixed.
Peter
Thanks, I'll push it shortly. Pavel
From: Luyao Huang <lhuang@redhat.com> Commit e105dc9 fix setting vcpus for offline domain, but forget check if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags. # virsh setvcpus test3 4 --live error: Failed to create controller cpu for group: No such file or directory Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006 Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4942712..c4d96bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + } + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob; @@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (ncpuinfo < 0) goto endjob; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto endjob; - } - if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0) goto endjob; -- 2.0.5
On 03/20/2015 10:39 PM, Pavel Hrdina wrote:
From: Luyao Huang <lhuang@redhat.com>
Commit e105dc9 fix setting vcpus for offline domain, but forget check if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.
# virsh setvcpus test3 4 --live error: Failed to create controller cpu for group: No such file or directory
Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006
Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4942712..c4d96bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
+ if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + } + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob; @@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (ncpuinfo < 0) goto endjob;
- if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto endjob; - } - if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0) goto endjob;
Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(), move this function just after qemuDomainObjBeginJob() maybe a good way to fix this issue. Also virDomainLiveConfigHelperMethod() may change flags, so it should be done more early (as soon as possible after set a lock to vm).
Luyao
On Tue, Mar 24, 2015 at 02:27:42PM +0800, lhuang wrote:
On 03/20/2015 10:39 PM, Pavel Hrdina wrote:
From: Luyao Huang <lhuang@redhat.com>
Commit e105dc9 fix setting vcpus for offline domain, but forget check if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.
# virsh setvcpus test3 4 --live error: Failed to create controller cpu for group: No such file or directory
Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006
Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4942712..c4d96bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
+ if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + } + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob; @@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (ncpuinfo < 0) goto endjob;
- if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto endjob; - } - if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0) goto endjob;
Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(), move this function just after qemuDomainObjBeginJob() maybe a good way to fix this issue. Also virDomainLiveConfigHelperMethod() may change flags, so it should be done more early (as soon as possible after set a lock to vm).
I've already sent a patch 7/6 to move that function. I realized that right after I've sent this series to mailing list. Pavel
Luyao
On 03/24/2015 05:31 PM, Pavel Hrdina wrote:
On Tue, Mar 24, 2015 at 02:27:42PM +0800, lhuang wrote:
On 03/20/2015 10:39 PM, Pavel Hrdina wrote:
From: Luyao Huang <lhuang@redhat.com>
Commit e105dc9 fix setting vcpus for offline domain, but forget check if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.
# virsh setvcpus test3 4 --live error: Failed to create controller cpu for group: No such file or directory
Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006
Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4942712..c4d96bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
+ if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + } + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob; @@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (ncpuinfo < 0) goto endjob;
- if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto endjob; - } - if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0) goto endjob; Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(), move this function just after qemuDomainObjBeginJob() maybe a good way to fix this issue. Also virDomainLiveConfigHelperMethod() may change flags, so it should be done more early (as soon as possible after set a lock to vm). I've already sent a patch 7/6 to move that function. I realized that right after I've sent this series to mailing list.
Pavel
Okay, I think i missed patch 7/6 when i looked these patches :) Thanks for your reply
Luyao
Luyao
On Fri, Mar 20, 2015 at 15:39:02 +0100, Pavel Hrdina wrote:
From: Luyao Huang <lhuang@redhat.com>
Commit e105dc9 fix setting vcpus for offline domain, but forget check if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.
# virsh setvcpus test3 4 --live error: Failed to create controller cpu for group: No such file or directory
Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006
Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
NACK, after the virDomainLiveConfigHelperMethod gets moved, this is not logner necessary as it contains the same code. Peter
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 1d8225c..9430ad9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(guest, config); + VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum); + VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum); if (config) flags |= VIR_DOMAIN_AFFECT_CONFIG; @@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) 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/20/2015 10:39 PM, Pavel Hrdina wrote:
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 1d8225c..9430ad9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(guest, config); + VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum); + VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
'maximum' should be used with 'config', and 'live' and 'maximum' are mutually exclusive
if (config) flags |= VIR_DOMAIN_AFFECT_CONFIG; @@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) 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; }
-- Regards, Yanbing Du
On Mon, Mar 23, 2015 at 11:33:50AM +0800, Yanbing Du wrote:
On 03/20/2015 10:39 PM, Pavel Hrdina wrote:
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 1d8225c..9430ad9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(guest, config); + VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum); + VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
'maximum' should be used with 'config', and 'live' and 'maximum' are mutually exclusive
Yes, you're right, I've definitely meant live instead of config. Good catch. Pavel
if (config) flags |= VIR_DOMAIN_AFFECT_CONFIG; @@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) 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; }
-- Regards, Yanbing Du
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 20, 2015 at 15:39:03 +0100, Pavel Hrdina wrote:
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 1d8225c..9430ad9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(guest, config); + VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum); + VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
As Yanbing pointed out, you want to make live and maximum exclusive. Additionally this changes semantics, as currently --maximum was possible if and only if --config was specified, which would make it exclusive with --current too. This is also implied in the man page. We have the following options: 1) Make --maximum imply --config and document that properly 2) Make --maximum mutualy exclusive with --current too 3) Allow --maximum and --current and document that it will fail for online domains I'm fine with either of those options Peter
On Tue, Mar 24, 2015 at 05:34:31PM +0100, Peter Krempa wrote:
On Fri, Mar 20, 2015 at 15:39:03 +0100, Pavel Hrdina wrote:
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 1d8225c..9430ad9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(guest, config); + VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum); + VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
As Yanbing pointed out, you want to make live and maximum exclusive.
Additionally this changes semantics, as currently --maximum was possible if and only if --config was specified, which would make it exclusive with --current too. This is also implied in the man page.
We have the following options:
1) Make --maximum imply --config and document that properly 2) Make --maximum mutualy exclusive with --current too 3) Allow --maximum and --current and document that it will fail for online domains
I'm fine with either of those options
There is also 4th option: specify that maximum requires config instead of make it mutually exclusive with current. This flag requirements are used by some of the other libvirt APIs. I'll create another set of macros to tell that some flags are required and send v2. Thanks for review. Pavel
Peter
We don't have to modify cpuset.mems on hosts without NUMA. It also fixes an error message that you get instead of success if you trying update vcpus of a guest on a host without NUMA. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4d96bd..eb86d68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4889,7 +4889,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, } } - if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST) && + virNumaIsAvailable()) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob; -- 2.0.5
On Fri, Mar 20, 2015 at 15:39:04 +0100, Pavel Hrdina wrote:
We don't have to modify cpuset.mems on hosts without NUMA. It also fixes an error message that you get instead of success if you trying update vcpus of a guest on a host without NUMA.
Could you add example of the error you are fixing here?
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4d96bd..eb86d68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4889,7 +4889,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, } }
- if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST) && + virNumaIsAvailable()) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob;
ACK. Peter
On Tue, Mar 24, 2015 at 05:38:37PM +0100, Peter Krempa wrote:
On Fri, Mar 20, 2015 at 15:39:04 +0100, Pavel Hrdina wrote:
We don't have to modify cpuset.mems on hosts without NUMA. It also fixes an error message that you get instead of success if you trying update vcpus of a guest on a host without NUMA.
Could you add example of the error you are fixing here?
Sure
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4d96bd..eb86d68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4889,7 +4889,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, } }
- if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) { + if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST) && + virNumaIsAvailable()) { if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) goto endjob;
ACK.
Peter
Thanks, will push it shortly. Pavel
participants (4)
-
lhuang -
Pavel Hrdina -
Peter Krempa -
Yanbing Du