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

[Eric Blake]

On 06/21/2010 03:47 PM, Justin Clift wrote:
> This patch adds a new --details option to the virsh pool-list
> command, making its output more useful to people who use virsh
> for significant lengths of time.
> 
> Addresses BZ # 605543
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=605543
> 
> +    /* Determine the total number of pools to list */
> +    numAllPools = numActivePools + numInactivePools;
> +    if (!numAllPools) {
> +      /* No pools to list, so cleanup and return */
> +      vshPrint(ctl, "%s\n", _("No matching pools to display"));
> +      return TRUE;
> +    }

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
$


>  
> -            qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter);
> +    /* Allocate memory for arrays of storage pool names and info */
> +    poolNames = vshCalloc(ctl, numAllPools, sizeof(char *));

Rather than sizeof(char *), I'd rather see sizeof(*poolNames), which
isolates us from problems if we ever change the type of poolNames.


> +    poolInfoTexts =
> +        vshCalloc(ctl, numAllPools, sizeof(struct poolInfoText *));

Likewise, sizeof(*poolInfoTexts)

> +
> +    /* Sort the storage pool names */
> +    qsort(poolNames, numAllPools, sizeof(char *), namesorter);

Again, sizeof(*poolNames)

Hmm, this changes existing output of --all, in that it now intermixes
lines for active and inactive pools, but I'm okay with that change (that
is, you swapped the primary sort key from being status over to being name).

>  
> +        /* 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.

> +        /* 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.

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

Missing _() around Capacity.


-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100622/3e3b38fa/attachment-0001.sig>