[libvirt] [PATCH 0/8] Simplify mutually exclusive argument handling in virsh
This patchset introduces a new macro that allows simple checking for mutually exclusive arguments in virsh and uses it in many occasions in virs. Peter Krempa (8): virsh: Introduce macros to reject mutually exclusive arguments virsh-snapshot: Refactor virsh snapshot-list virsh-host: Refactor cmdFreecell virsh-domain: Fix flag name in error message to match the check virsh-snapshot: Refactor cmdSnapshotCurrent virsh-snapshot: Use the mutually exclusive params macro in cmdSnapshotEdit virsh-snapshot: Simplify cleanup path in cmdSnapshotEdit virsh-domain: Simplify usage of --current, --live and --config flags po/POTFILES.in | 1 + tools/virsh-domain.c | 262 +++++++++++++++++++++---------------------------- tools/virsh-host.c | 55 +++++------ tools/virsh-snapshot.c | 181 +++++++++++++--------------------- tools/virsh.h | 42 ++++++++ 5 files changed, 248 insertions(+), 293 deletions(-) -- 1.8.1.1
This patch adds three macros to the virsh source tree that help to easily check for mutually exclusive parameters. VSH_EXCLUSIVE_OPTIONS_EXPR has four arguments, two expressions to check and two names of the parameters to print in the message. VSH_EXCLUSIVE_OPTIONS is more specific and check the command structure for the parameters using vshCommandOptBool. VSH_EXCLUSIVE_OPTIONS_VAR is meant to check boolean variables with the same name as the parameters. --- po/POTFILES.in | 1 + tools/virsh.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index bd2c02e..7a2da8f 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -204,6 +204,7 @@ src/xenxs/xen_xm.c tools/console.c tools/libvirt-guests.sh.in tools/virsh.c +tools/virsh.h tools/virsh-domain-monitor.c tools/virsh-domain.c tools/virsh-edit.c diff --git a/tools/virsh.h b/tools/virsh.h index fec9785..410d859 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -375,4 +375,46 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, # define realloc use_vshRealloc_instead_of_realloc # define strdup use_vshStrdup_instead_of_strdup +/* Macros to help dealing with mutually exclusive options. */ + +/* VSH_EXCLUSIVE_OPTIONS_EXPR: + * + * @NAME1: String containing the name of the option. + * @EXPR1: Expression to validate the variable (boolean variable) + * @NAME2: String containing the name of the option. + * @EXPR2: Expression to validate the variable (boolean variable) + * + * Reject mutually exclusive command options in virsh. Use the + * provided expression to check the variables. + */ +# define VSH_EXCLUSIVE_OPTIONS_EXPR(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && (EXPR2)) { \ + vshError(ctl, _("Options --%s and --%s are mutually exclusive"), \ + NAME1, NAME2); \ + return false; \ + } + +/* VSH_EXCLUSIVE_OPTIONS: + * + * @NAME1: String containing the name of the option. + * @NAME2: String containing the name of the option. + * + * Reject mutually exclusive command options in virsh. Use the + * vshCommandOptBool call to request them. + */ +# define VSH_EXCLUSIVE_OPTIONS(NAME1, NAME2) \ + VSH_EXCLUSIVE_OPTIONS_EXPR(NAME1, vshCommandOptBool(cmd, NAME1), \ + NAME2, vshCommandOptBool(cmd, NAME2)) + +/* VSH_EXCLUSIVE_OPTIONS_VAR: + * + * @VARNAME1: Boolean variable containing the value of the option of same name + * @VARNAME2: Boolean variable containing the value of the option of same name + * + * Reject mutually exclusive command options in virsh. Check in variables that + * contain the value and have same name as the option. + */ +# define VSH_EXCLUSIVE_OPTIONS_VAR(VARNAME1, VARNAME2) \ + VSH_EXCLUSIVE_OPTIONS_EXPR(#VARNAME1, VARNAME1, #VARNAME2, VARNAME2) + #endif /* VIRSH_H */ -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
This patch adds three macros to the virsh source tree that help to easily check for mutually exclusive parameters.
VSH_EXCLUSIVE_OPTIONS_EXPR has four arguments, two expressions to check and two names of the parameters to print in the message.
VSH_EXCLUSIVE_OPTIONS is more specific and check the command structure for the parameters using vshCommandOptBool.
VSH_EXCLUSIVE_OPTIONS_VAR is meant to check boolean variables with the same name as the parameters. --- po/POTFILES.in | 1 + tools/virsh.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)
+++ b/tools/virsh.h @@ -375,4 +375,46 @@ char *_vshStrdup(vshControl *ctl, const char *s, const char *filename, # define realloc use_vshRealloc_instead_of_realloc # define strdup use_vshStrdup_instead_of_strdup
+/* Macros to help dealing with mutually exclusive options. */ + +/* VSH_EXCLUSIVE_OPTIONS_EXPR: + * + * @NAME1: String containing the name of the option. + * @EXPR1: Expression to validate the variable (boolean variable) + * @NAME2: String containing the name of the option. + * @EXPR2: Expression to validate the variable (boolean variable) + * + * Reject mutually exclusive command options in virsh. Use the + * provided expression to check the variables. + */ +# define VSH_EXCLUSIVE_OPTIONS_EXPR(NAME1, EXPR1, NAME2, EXPR2) \ + if ((EXPR1) && (EXPR2)) { \ + vshError(ctl, _("Options --%s and --%s are mutually exclusive"), \ + NAME1, NAME2); \ + return false; \
This does an early return, and therefore the macro must be used before anything that needs cleanup. At any rate, these look sane to me. ACK if you add a sentence along those lines. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Simplify error handling and mutually exlusive option checking. --- tools/virsh-snapshot.c | 121 ++++++++++++++++++------------------------------- 1 file changed, 45 insertions(+), 76 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index ed41014..d4aa4de 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1571,66 +1571,34 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; unsigned int flags = 0; - bool show_parent = false; int i; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; char *doc = NULL; virDomainSnapshotPtr snapshot = NULL; char *state = NULL; - char *parent = NULL; long long creation_longlong; time_t creation_time_t; char timestr[100]; struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); bool name = vshCommandOptBool(cmd, "name"); - const char *from = NULL; + bool from = vshCommandOptBool(cmd, "from"); + bool parent = vshCommandOptBool(cmd, "parent"); + bool roots = vshCommandOptBool(cmd, "roots"); + bool current = vshCommandOptBool(cmd, "current"); + const char *from_snap = NULL; + char *parent_snap = NULL; virDomainSnapshotPtr start = NULL; vshSnapshotListPtr snaplist = NULL; - if (tree && name) { - vshError(ctl, "%s", - _("--tree and --name are mutually exclusive")); - return false; - } - - dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; + VSH_EXCLUSIVE_OPTIONS_VAR(tree, name); + VSH_EXCLUSIVE_OPTIONS_VAR(parent, roots); + VSH_EXCLUSIVE_OPTIONS_VAR(parent, tree); + VSH_EXCLUSIVE_OPTIONS_VAR(roots, tree); + VSH_EXCLUSIVE_OPTIONS_VAR(roots, from); + VSH_EXCLUSIVE_OPTIONS_VAR(roots, current); - if ((vshCommandOptBool(cmd, "from") || - vshCommandOptBool(cmd, "current")) && - vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from) < 0) - goto cleanup; - - if (vshCommandOptBool(cmd, "parent")) { - if (vshCommandOptBool(cmd, "roots")) { - vshError(ctl, "%s", - _("--parent and --roots are mutually exclusive")); - goto cleanup; - } - if (tree) { - vshError(ctl, "%s", - _("--parent and --tree are mutually exclusive")); - goto cleanup; - } - show_parent = true; - } else if (vshCommandOptBool(cmd, "roots")) { - if (tree) { - vshError(ctl, "%s", - _("--roots and --tree are mutually exclusive")); - goto cleanup; - } - if (from) { - vshError(ctl, "%s", - vshCommandOptBool(cmd, "current") ? - _("--roots and --current are mutually exclusive") : - _("--roots and --from are mutually exclusive")); - goto cleanup; - } - flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; - } #define FILTER(option, flag) \ do { \ if (vshCommandOptBool(cmd, option)) { \ @@ -1638,7 +1606,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) vshError(ctl, \ _("--%s and --tree are mutually exclusive"), \ option); \ - goto cleanup; \ + return false; \ } \ flags |= VIR_DOMAIN_SNAPSHOT_LIST_ ## flag; \ } \ @@ -1653,28 +1621,36 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) FILTER("external", EXTERNAL); #undef FILTER - if (vshCommandOptBool(cmd, "metadata")) { + if (roots) + flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + + if (vshCommandOptBool(cmd, "metadata")) flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; - } - if (vshCommandOptBool(cmd, "no-metadata")) { + + if (vshCommandOptBool(cmd, "no-metadata")) flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA; - } if (vshCommandOptBool(cmd, "descendants")) { - if (!from) { + if (!from && !current) { vshError(ctl, "%s", _("--descendants requires either --from or --current")); - goto cleanup; + return false; } flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } - if ((snaplist = vshSnapshotListCollect(ctl, dom, start, flags, - tree)) == NULL) + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if ((from || current) && + vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from_snap) < 0) + goto cleanup; + + if (!(snaplist = vshSnapshotListCollect(ctl, dom, start, flags, tree))) goto cleanup; if (!tree && !name) { - if (show_parent) + if (parent) vshPrintExtra(ctl, " %-20s %-25s %-15s %s", _("Name"), _("Creation Time"), _("State"), _("Parent")); @@ -1682,12 +1658,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, " %-20s %-25s %s", _("Name"), _("Creation Time"), _("State")); vshPrintExtra(ctl, "\n" -"------------------------------------------------------------\n"); - } - - if (!snaplist->nsnaps) { - ret = true; - goto cleanup; + "------------------------------" + "------------------------------\n"); } if (tree) { @@ -1705,7 +1677,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) const char *snap_name; /* free up memory from previous iterations of the loop */ - VIR_FREE(parent); + VIR_FREE(parent_snap); VIR_FREE(state); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); @@ -1716,25 +1688,24 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) assert(snap_name); if (name) { + /* just print the snapshot name */ vshPrint(ctl, "%s\n", snap_name); continue; } - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) + if (!(doc = virDomainSnapshotGetXMLDesc(snapshot, 0))) continue; - xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt); - if (!xml) + if (!(xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt))) continue; - if (show_parent) - parent = virXPathString("string(/domainsnapshot/parent/name)", - ctxt); + if (parent) + parent_snap = virXPathString("string(/domainsnapshot/parent/name)", + ctxt); - state = virXPathString("string(/domainsnapshot/state)", ctxt); - if (state == NULL) + if (!(state = virXPathString("string(/domainsnapshot/state)", ctxt))) continue; + if (virXPathLongLong("string(/domainsnapshot/creationTime)", ctxt, &creation_longlong) < 0) continue; @@ -1749,7 +1720,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (parent) vshPrint(ctl, " %-20s %-25s %-15s %s\n", - snap_name, timestr, state, parent); + snap_name, timestr, state, parent_snap); else vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state); } @@ -1759,15 +1730,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) cleanup: /* this frees up memory from the last iteration of the loop */ vshSnapshotListFree(snaplist); - VIR_FREE(parent); + VIR_FREE(parent_snap); VIR_FREE(state); - if (start) - virDomainSnapshotFree(start); + virDomainSnapshotFree(start); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - if (dom) - virDomainFree(dom); + virDomainFree(dom); return ret; } -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
Simplify error handling and mutually exlusive option checking.
s/exlusive/exclusive/
--- tools/virsh-snapshot.c | 121 ++++++++++++++++++------------------------------- 1 file changed, 45 insertions(+), 76 deletions(-)
ACK. It might have been nicer to split into two patches - one to rename existing variables with no other changes, and the second to use the new macros; but it's not worth splitting now. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the new helpers to determine mutually exclusive options and touch up some parts to simplify the code. --- tools/virsh-host.c | 55 +++++++++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 32 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 4345839..22bcd6c 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -159,35 +159,29 @@ static const vshCmdOptDef opts_freecell[] = { static bool cmdFreecell(vshControl *ctl, const vshCmd *cmd) { - bool func_ret = false; - int ret; - int cell = -1, cell_given; - unsigned long long memory; + bool ret = false; + int cell = -1; + unsigned long long memory = 0; xmlNodePtr *nodes = NULL; unsigned long nodes_cnt; unsigned long *nodes_id = NULL; unsigned long long *nodes_free = NULL; - int all_given; + bool all = vshCommandOptBool(cmd, "all"); + bool cellno = vshCommandOptBool(cmd, "cellno"); int i; char *cap_xml = NULL; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; - if ((cell_given = vshCommandOptInt(cmd, "cellno", &cell)) < 0) { - vshError(ctl, "%s", _("cell number has to be a number")); - goto cleanup; - } - all_given = vshCommandOptBool(cmd, "all"); + VSH_EXCLUSIVE_OPTIONS_VAR(all, cellno); - if (all_given && cell_given) { - vshError(ctl, "%s", _("--cellno and --all are mutually exclusive. " - "Please choose only one.")); - goto cleanup; + if (cellno && vshCommandOptInt(cmd, "cellno", &cell) < 0) { + vshError(ctl, "%s", _("cell number has to be a number")); + return false; } - if (all_given) { - cap_xml = virConnectGetCapabilities(ctl->conn); - if (!cap_xml) { + if (all) { + if (!(cap_xml = virConnectGetCapabilities(ctl->conn))) { vshError(ctl, "%s", _("unable to get node capabilities")); goto cleanup; } @@ -197,6 +191,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) vshError(ctl, "%s", _("unable to get node capabilities")); goto cleanup; } + nodes_cnt = virXPathNodeSet("/capabilities/host/topology/cells/cell", ctxt, &nodes); @@ -219,15 +214,14 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) } VIR_FREE(val); nodes_id[i]=id; - ret = virNodeGetCellsFreeMemory(ctl->conn, &(nodes_free[i]), id, 1); - if (ret != 1) { + if (virNodeGetCellsFreeMemory(ctl->conn, &(nodes_free[i]), + id, 1) != 1) { vshError(ctl, _("failed to get free memory for NUMA node " "number: %lu"), id); goto cleanup; } } - memory = 0; for (cell = 0; cell < nodes_cnt; cell++) { vshPrint(ctl, "%5lu: %10llu KiB\n", nodes_id[cell], (nodes_free[cell]/1024)); @@ -237,23 +231,20 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, "--------------------\n"); vshPrintExtra(ctl, "%5s: %10llu KiB\n", _("Total"), memory/1024); } else { - if (!cell_given) { - memory = virNodeGetFreeMemory(ctl->conn); - if (memory == 0) + if (cellno) { + if (virNodeGetCellsFreeMemory(ctl->conn, &memory, cell, 1) != 1) goto cleanup; - } else { - ret = virNodeGetCellsFreeMemory(ctl->conn, &memory, cell, 1); - if (ret != 1) + + vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024)); + } else { + if ((memory = virNodeGetFreeMemory(ctl->conn)) == 0) goto cleanup; - } - if (cell == -1) vshPrint(ctl, "%s: %llu KiB\n", _("Total"), (memory/1024)); - else - vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024)); + } } - func_ret = true; + ret = true; cleanup: xmlXPathFreeContext(ctxt); @@ -262,7 +253,7 @@ cleanup: VIR_FREE(nodes_free); VIR_FREE(nodes_id); VIR_FREE(cap_xml); - return func_ret; + return ret; } /* -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
Use the new helpers to determine mutually exclusive options and touch up some parts to simplify the code. --- tools/virsh-host.c | 55 +++++++++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 32 deletions(-)
- if ((cell_given = vshCommandOptInt(cmd, "cellno", &cell)) < 0) { - vshError(ctl, "%s", _("cell number has to be a number")); - goto cleanup; - } - all_given = vshCommandOptBool(cmd, "all"); + VSH_EXCLUSIVE_OPTIONS_VAR(all, cellno);
- if (all_given && cell_given) { - vshError(ctl, "%s", _("--cellno and --all are mutually exclusive. " - "Please choose only one.")); - goto cleanup; + if (cellno && vshCommandOptInt(cmd, "cellno", &cell) < 0) {
The 'cellno &&' portion is extra; we didn't need it before, so I don't know why you added it here. vshCommandOptInt returns 0 if --cellno was not provided, since it is not a mandatory option.
@@ -237,23 +231,20 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, "--------------------\n"); vshPrintExtra(ctl, "%5s: %10llu KiB\n", _("Total"), memory/1024); } else { - if (!cell_given) { - memory = virNodeGetFreeMemory(ctl->conn); - if (memory == 0) + if (cellno) {
Indentation is off.
+ if (virNodeGetCellsFreeMemory(ctl->conn, &memory, cell, 1) != 1) goto cleanup; - } else { - ret = virNodeGetCellsFreeMemory(ctl->conn, &memory, cell, 1); - if (ret != 1) + + vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024)); + } else {
and again. ACK if you either explain the added conjunct or remove it, and if you fix the whitespace. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/21/13 04:48, Eric Blake wrote:
On 03/07/2013 05:53 AM, Peter Krempa wrote:
Use the new helpers to determine mutually exclusive options and touch up some parts to simplify the code. --- tools/virsh-host.c | 55 +++++++++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 32 deletions(-)
- if ((cell_given = vshCommandOptInt(cmd, "cellno", &cell)) < 0) { - vshError(ctl, "%s", _("cell number has to be a number")); - goto cleanup; - } - all_given = vshCommandOptBool(cmd, "all"); + VSH_EXCLUSIVE_OPTIONS_VAR(all, cellno);
- if (all_given && cell_given) { - vshError(ctl, "%s", _("--cellno and --all are mutually exclusive. " - "Please choose only one.")); - goto cleanup; + if (cellno && vshCommandOptInt(cmd, "cellno", &cell) < 0) {
The 'cellno &&' portion is extra; we didn't need it before, so I don't know why you added it here. vshCommandOptInt returns 0 if --cellno was not provided, since it is not a mandatory option.
I used it there to avoid the call to vshCommandOptInt in case it won't parse anything as the cellno parameter is missing anyways. It is not strictly needed but it's a optimization so I'll leave it in.
ACK if you either explain the added conjunct or remove it, and if you fix the whitespace.
Peter
The check is done on the "--paused" flag by the error message stated "--started" --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 96dd4fa..8dad85d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3792,7 +3792,7 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) * step fails, but the define step will always fail on invalid * flags, so we reject it up front to avoid looping. */ if (define_flags == (VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED)) { - vshError(ctl, "%s", _("--running and --saved are mutually exclusive")); + vshError(ctl, "%s", _("--running and --paused are mutually exclusive")); return false; } -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
The check is done on the "--paused" flag by the error message stated
s/by/but/
"--started" --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 96dd4fa..8dad85d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3792,7 +3792,7 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) * step fails, but the define step will always fail on invalid * flags, so we reject it up front to avoid looping. */ if (define_flags == (VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED)) { - vshError(ctl, "%s", _("--running and --saved are mutually exclusive")); + vshError(ctl, "%s", _("--running and --paused are mutually exclusive")); return false; }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the exclusive parameter checker and touch up some parts to simplify code. --- tools/virsh-snapshot.c | 46 +++++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index d4aa4de..78db178 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -712,9 +712,10 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "security-info")) flags |= VIR_DOMAIN_XML_SECURE; - dom = vshCommandOptDomain(ctl, cmd, &domname); - if (dom == NULL) - goto cleanup; + VSH_EXCLUSIVE_OPTIONS("name", "snapshotname"); + + if (!(dom = vshCommandOptDomain(ctl, cmd, &domname))) + return false; if (vshCommandOptStringReq(ctl, cmd, "snapshotname", &snapshotname) < 0) goto cleanup; @@ -724,52 +725,48 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) flags = (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT); - if (vshCommandOptBool(cmd, "name")) { - vshError(ctl, "%s", - _("--name and snapshotname are mutually exclusive")); - goto cleanup; - } - snapshot = virDomainSnapshotLookupByName(dom, snapshotname, 0); - if (snapshot == NULL) + if (!(snapshot = virDomainSnapshotLookupByName(dom, snapshotname, 0))) goto cleanup; + xml = virDomainSnapshotGetXMLDesc(snapshot, VIR_DOMAIN_XML_SECURE); if (!xml) goto cleanup; + /* strstr is safe here, since xml came from libvirt API and not user */ if (strstr(xml, "<state>disk-snapshot</state>")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; - snapshot2 = virDomainSnapshotCreateXML(dom, xml, flags); - if (snapshot2 == NULL) + + if (!(snapshot2 = virDomainSnapshotCreateXML(dom, xml, flags))) goto cleanup; + virDomainSnapshotFree(snapshot2); vshPrint(ctl, _("Snapshot %s set as current"), snapshotname); ret = true; goto cleanup; } - current = virDomainHasCurrentSnapshot(dom, 0); - if (current < 0) { + if ((current = virDomainHasCurrentSnapshot(dom, 0)) < 0) goto cleanup; - } else if (!current) { + + if (!current) { vshError(ctl, _("domain '%s' has no current snapshot"), domname); goto cleanup; } else { - const char *name = NULL; - if (!(snapshot = virDomainSnapshotCurrent(dom, 0))) goto cleanup; if (vshCommandOptBool(cmd, "name")) { - name = virDomainSnapshotGetName(snapshot); - if (!name) + const char *name; + if (!(name = virDomainSnapshotGetName(snapshot))) goto cleanup; + + vshPrint(ctl, "%s", name); } else { - xml = virDomainSnapshotGetXMLDesc(snapshot, flags); - if (!xml) + if (!(xml = virDomainSnapshotGetXMLDesc(snapshot, flags))) goto cleanup; - } - vshPrint(ctl, "%s", name ? name : xml); + vshPrint(ctl, "%s", xml); + } } ret = true; @@ -780,8 +777,7 @@ cleanup: VIR_FREE(xml); if (snapshot) virDomainSnapshotFree(snapshot); - if (dom) - virDomainFree(dom); + virDomainFree(dom); return ret; } -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
Use the exclusive parameter checker and touch up some parts to simplify code. --- tools/virsh-snapshot.c | 46 +++++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 25 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tools/virsh-snapshot.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 78db178..3add7fb 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -584,11 +584,7 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) bool rename_okay = vshCommandOptBool(cmd, "rename"); bool clone_okay = vshCommandOptBool(cmd, "clone"); - if (rename_okay && clone_okay) { - vshError(ctl, "%s", - _("--rename and --clone are mutually exclusive")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_EXPR("rename", rename_okay, "clone", clone_okay) if (vshCommandOptBool(cmd, "current") && vshCommandOptBool(cmd, "snapshotname")) -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
--- tools/virsh-snapshot.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
ACK.
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 78db178..3add7fb 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -584,11 +584,7 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) bool rename_okay = vshCommandOptBool(cmd, "rename"); bool clone_okay = vshCommandOptBool(cmd, "clone");
- if (rename_okay && clone_okay) { - vshError(ctl, "%s", - _("--rename and --clone are mutually exclusive")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_EXPR("rename", rename_okay, "clone", clone_okay)
if (vshCommandOptBool(cmd, "current") && vshCommandOptBool(cmd, "snapshotname"))
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tools/virsh-snapshot.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 3add7fb..e1f66e3 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -590,9 +590,8 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) vshCommandOptBool(cmd, "snapshotname")) define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; - dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; if (vshLookupSnapshot(ctl, cmd, "snapshotname", false, dom, &snapshot, &name) < 0) @@ -651,8 +650,7 @@ cleanup: virDomainSnapshotFree(edited); if (snapshot) virDomainSnapshotFree(snapshot); - if (dom) - virDomainFree(dom); + virDomainFree(dom); return ret; } -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
--- tools/virsh-snapshot.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch uses the new helper to avoid the more complex check for domain state modification flags. --- tools/virsh-domain.c | 260 ++++++++++++++++++++++----------------------------- 1 file changed, 111 insertions(+), 149 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8dad85d..4c00db3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1068,18 +1068,15 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) bool live = vshCommandOptBool(cmd, "live"); bool ret = false; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) goto cleanup; @@ -1256,18 +1253,15 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -2570,18 +2564,15 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) virNetDevBandwidthRate inbound, outbound; int i; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; @@ -4146,18 +4137,15 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -5618,21 +5606,18 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool query = false; /* Query mode if no cpulist */ unsigned int flags = 0; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - /* neither option is specified */ - if (!live && !config) - flags = -1; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + /* neither option is specified */ + if (!current && !live && !config) + flags = -1; if (vshCommandOptStringReq(ctl, cmd, "cpulist", &cpulist) < 0) return false; @@ -5855,21 +5840,18 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd) bool query = false; /* Query mode if no cpulist */ unsigned int flags = 0; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - /* neither option is specified */ - if (!live && !config) - flags = -1; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + /* neither option is specified */ + if (!current && !live && !config) + flags = -1; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -6058,21 +6040,18 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); unsigned int flags = 0; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - /* neither option is specified */ - if (!live && !config && !maximum) - flags = -1; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + /* neither option is specified */ + if (!current && !live && !config && !maximum) + flags = -1; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -6766,18 +6745,13 @@ cmdDesc(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) bool ret = false; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } - flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -7189,21 +7163,18 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); unsigned int flags = 0; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - /* neither option is specified */ - if (!live && !config) - flags = -1; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + /* neither option is specified */ + if (!current && !live && !config) + flags = -1; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -7295,20 +7266,18 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); unsigned int flags = VIR_DOMAIN_MEM_MAXIMUM; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - /* neither option is specified */ - if (!live && !config) - flags = -1; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) + flags = VIR_DOMAIN_AFFECT_CURRENT; + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + /* neither option is specified */ + if (!current && !live && !config) + flags = -1; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -7619,18 +7588,15 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) bool live = vshCommandOptBool(cmd, "live"); const char *mode = NULL; - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -10318,19 +10284,15 @@ cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) action = "update"; } - if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } - flags = VIR_DOMAIN_AFFECT_CURRENT; - } else { - if (config) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + if (current) + flags = VIR_DOMAIN_AFFECT_CURRENT; + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; if (force) flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE; -- 1.8.1.1
On 03/07/2013 05:53 AM, Peter Krempa wrote:
This patch uses the new helper to avoid the more complex check for domain state modification flags. --- tools/virsh-domain.c | 260 ++++++++++++++++++++++----------------------------- 1 file changed, 111 insertions(+), 149 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8dad85d..4c00db3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1068,18 +1068,15 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) bool live = vshCommandOptBool(cmd, "live"); bool ret = false;
- if (current) { - if (live || config) { - vshError(ctl, "%s", _("--current must be specified exclusively")); - return false; - } + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (current) flags = VIR_DOMAIN_AFFECT_CURRENT;
We already know that VIR_DOMAIN_AFFECT_CURRENT == 0; if flags was pre-initialized to 0, this assignment is redundant. But I guess it doesn't hurt.
+ /* neither option is specified */ + if (!current && !live && !config && !maximum)
Generally, 'neither option' implies a choice between 2, but here you are choosing between 4. A better wording might be 'no options were specified'. As my complaints don't affect correctness of your patch as-is, ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/07/13 13:52, Peter Krempa wrote:
This patchset introduces a new macro that allows simple checking for mutually exclusive arguments in virsh and uses it in many occasions in virs.
Peter Krempa (8): virsh: Introduce macros to reject mutually exclusive arguments virsh-snapshot: Refactor virsh snapshot-list virsh-host: Refactor cmdFreecell virsh-domain: Fix flag name in error message to match the check virsh-snapshot: Refactor cmdSnapshotCurrent virsh-snapshot: Use the mutually exclusive params macro in cmdSnapshotEdit virsh-snapshot: Simplify cleanup path in cmdSnapshotEdit virsh-domain: Simplify usage of --current, --live and --config flags
po/POTFILES.in | 1 + tools/virsh-domain.c | 262 +++++++++++++++++++++---------------------------- tools/virsh-host.c | 55 +++++------ tools/virsh-snapshot.c | 181 +++++++++++++--------------------- tools/virsh.h | 42 ++++++++ 5 files changed, 248 insertions(+), 293 deletions(-)
Ping?
participants (2)
-
Eric Blake -
Peter Krempa