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

Re: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list



On 06/23/2010 03:44 AM, Eric Blake wrote:
<snip>
This is a change in behavior, even when the user didn't specify
--details.  Might it cause any backward compatibility issues in
scripting, or are we okay with the change?

$ old-virsh -c test:///default pool-list --inactive
Name                 State      Autostart
-----------------------------------------
$ tools/virsh -c test:///default pool-list --inactive
No matching pools to display
$

Good point. I can wrap a conditional if construct around that, to maintain backwards compatibility, but also have nicer text going forwards.

Kind of thinking it may not be worth that effort through, and it may be better to just use the old output format for consistency when no matching pools are found.

There's possible a better way, not fully conceived yet (!).

I've been thinking about adding some kind of user level configuration file in (ie.) ~/.virsh/config. This could change "virsh" wide options for the running session, things like changing output format of error messages, whether the "--all" option is assumed by default for list commands (yes please), etc.

Introducing that would give us a way to address/fix some of the wider virsh issues, but maintain backwards compatibility for the apps/scripts that need it. (ie with an option like "use_old_error_outputs = yes")


<snips>
Likewise, sizeof(*poolInfoTexts)
Again, sizeof(*poolNames)

No worries.  I'll resubmit the fix.


+        /* Allocate and clear memory for one row of pool info */
+        poolInfoTexts[i] = vshCalloc(ctl, 1, sizeof(struct poolInfoText));

Why did we allocate an array of poolInfoText*, only to then allocate
each poolInfoText?  I'd rather see poolInfoTexts be (struct
poolInfoText*) than (struct poolInfoText**), so that allocating the
array up front is good enough; we don't have to do further allocation
each time through the loop.

Cool, it was probably me just not mapping something out completely. I'll look at that too.


+        /* Collect further extended information about the pool */
+        if (virStoragePoolGetInfo(pool,&info) != 0) {
+            /* Something went wrong retrieving pool info, cope with it */
+            vshError(ctl, "%s", _("Could not retrieve pool information"));
+            poolInfoTexts[i]->state = vshStrdup(ctl, _("unknown"));
+            poolInfoTexts[i]->capacity = vshStrdup(ctl, _("unknown"));
+            poolInfoTexts[i]->allocation = vshStrdup(ctl, _("unknown"));
+            poolInfoTexts[i]->available = vshStrdup(ctl, _("unknown"));

We could surround the last three vshStrdup with if (details), to avoid
malloc pressure for the "unknown" answer that would not be printed.

Yep, I'll look at that too.


+    /* Display the header */
+    vshPrint(ctl, outputStr, _("Name"), _("State"), _("Autostart"),
+             _("Persistent"), ("Capacity"), _("Allocation"), _("Available"));

Missing _() around Capacity.

Oops, thanks. :)

Additionally, after looking at this again a day later I'm also thinking these two tweaks would be better:

a) Moving the "function_ret = FALSE" out of several:

     if (ret < 0) {
         /* An error occurred creating the string, return */
         function_ret = FALSE;
         goto asprintf_failure;
     }

  ... into the asprintf_failure: code block itself.  i.e:

     if (ret < 0) {
         /* An error occurred creating the string, return */
         goto asprintf_failure;
     }

b) The code fragment calculates the length of the "persistent" string is also badly placed:

    /* Keep the length of persistent string if longest so far */
    stringLength = strlen(poolInfoTexts[i]->persistent);
    if (stringLength > persistStrLength)
        persistStrLength = stringLength;

It needs to be moved outwards by two code blocks, to be with the code that checks the autostart string length.

I'll resubmit with all of the above fixed. Waiting for the online discussion about the error output format though. :)

Regards and best wishes,

Justin Clift

--
Salasaga  -  Open Source eLearning IDE
              http://www.salasaga.org


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