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

[Justin Clift]

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