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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list