Re: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)