[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/7] virsh-domain: Refactor cmdVcpucount and fix output on inactive domains



On 04/16/13 00:44, Eric Blake wrote:
On 04/15/2013 09:11 AM, Peter Krempa wrote:
This patch factors out the vCPU count retrieval including fallback means into
vshCPUCountCollect() and removes the duplicated code to retrieve individual
counts.

The --current flag (this flag is assumed by default) now works also with
--maximum or --active without the need to explicitly specify the state of the
domain that is requested.

This patch also fixes the output of "virsh vcpucount domain" on inactive
domains:

Before:
$ virsh vcpucount domain
maximum      config         4
error: Requested operation is not valid: domain is not running
current      config         4
error: Requested operation is not valid: domain is not running

After:
$virsh vcpucount domain
maximum      config         4
current      config         4

Looks nicer; and I don't think we ever promised that there would always
be exactly four lines of output.  What about the converse case, of
attempting to query the config of a transient domain?  Should you also
clean up that output to avoid error messages?

---
  tools/virsh-domain.c | 258 +++++++++++++++++++++++----------------------------
  1 file changed, 115 insertions(+), 143 deletions(-)


  cmdVcpucount(vshControl *ctl, const vshCmd *cmd)
  {
      virDomainPtr dom;
-    bool ret = true;
+    bool ret = false;
      bool maximum = vshCommandOptBool(cmd, "maximum");
      bool active = vshCommandOptBool(cmd, "active");
      bool config = vshCommandOptBool(cmd, "config");
      bool live = vshCommandOptBool(cmd, "live");
      bool current = vshCommandOptBool(cmd, "current");
      bool all = maximum + active + current + config + live == 0;
-    int count;
+    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;

-    /* We want one of each pair of mutually exclusive options; that
-     * is, use of flags requires exactly two options.  We reject the
-     * use of more than 2 flags later on.  */
-    if (maximum + active + current + config + live == 1) {
-        if (maximum || active) {
-            vshError(ctl,
-                     _("when using --%s, one of --config, --live, or --current "
-                       "must be specified"),
-                     maximum ? "maximum" : "active");
-        } else {
-            vshError(ctl,
-                     _("when using --%s, either --maximum or --active must be "
-                       "specified"),
-                     (current ? "current" : config ? "config" : "live"));
-        }
-        return false;
-    }
+    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);

NACK to this line, at least in this location.  It prevents...

+    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+    VSH_EXCLUSIVE_OPTIONS_VAR(active, maximum);

      /* Backwards compatibility: prior to 0.9.4,
       * VIR_DOMAIN_AFFECT_CURRENT was unsupported, and --current meant

...this backwards-compatibility hack, where '--current --live' must
behave like the modern '--active --live'.

Hmm, yeah, I didn't port the compatibility code correctly here. Even if the check is moved, the compatibility code needs to be updated.

Peter


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]