[libvirt] [PATCHv2 1/2] virsh: add new --details option to pool-list
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 --- Output from the new option (hopefully this doesn't wrap): virsh # pool-list Name State Autostart ----------------------------------------- default active yes image_dir active yes virsh # pool-list --all Name State Autostart ----------------------------------------- default active yes image_dir active yes tmp inactive no virsh # pool-list --details Name State Autostart Persistent Capacity Allocation Available -------------------------------------------------------------------------------------- default running yes yes 1.79 TB 1.47 TB 326.02 GB image_dir running yes yes 1.79 TB 1.47 TB 326.02 GB virsh # pool-list --all --details Name State Autostart Persistent Capacity Allocation Available -------------------------------------------------------------------------------------- default running yes yes 1.79 TB 1.47 TB 326.02 GB image_dir running yes yes 1.79 TB 1.47 TB 326.02 GB tmp inactive no yes - - - virsh # Much more practical than running pool-info individually on each pool. tools/virsh.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 6 ++- 2 files changed, 119 insertions(+), 17 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d8d2220..afa84e6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4882,14 +4882,17 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; static int cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + virStoragePoolInfo info; int inactive = vshCommandOptBool(cmd, "inactive"); int all = vshCommandOptBool(cmd, "all"); + int details = vshCommandOptBool(cmd, "details"); int active = !inactive || all ? 1 : 0; int maxactive = 0, maxinactive = 0, i; char **activeNames = NULL, **inactiveNames = NULL; @@ -4937,36 +4940,114 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter); } } - vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Display the appropriate heading */ + if (details) { + vshPrintExtra(ctl, "%-20s %-10s %-10s %-11s %-9s %-11s %-10s\n", + _("Name"), _("State"), _("Autostart"), _("Persistent"), + _("Capacity"), _("Allocation"), _("Available")); + vshPrintExtra(ctl, + "--------------------------------------------------------------------------------------\n"); + } else { + vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), + _("Autostart")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + } for (i = 0; i < maxactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, activeNames[i]); - const char *autostartStr; - int autostart = 0; + const char *autostartStr, *persistentStr, *stateStr = NULL; + int autostart = 0, persistent = 0; /* this kind of work with pools is not atomic operation */ + virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, activeNames[i]); if (!pool) { VIR_FREE(activeNames[i]); continue; } + /* Retrieve the pool autostart status */ if (virStoragePoolGetAutostart(pool, &autostart) < 0) autostartStr = _("no autostart"); else autostartStr = autostart ? _("yes") : _("no"); - vshPrint(ctl, "%-20s %-10s %-10s\n", - virStoragePoolGetName(pool), - _("active"), - autostartStr); + /* If requested, collect the extended information for this pool */ + if (details) { + if (virStoragePoolGetInfo(pool, &info) != 0) { + vshError(ctl, "%s", _("Could not retrieve pool information")); + VIR_FREE(activeNames[i]); + continue; + } + + /* Decide which state string to display */ + switch (info.state) { + case VIR_STORAGE_POOL_INACTIVE: + stateStr = _("inactive"); + break; + case VIR_STORAGE_POOL_BUILDING: + stateStr = _("building"); + break; + case VIR_STORAGE_POOL_RUNNING: + stateStr = _("running"); + break; + case VIR_STORAGE_POOL_DEGRADED: + stateStr = _("degraded"); + break; + case VIR_STORAGE_POOL_INACCESSIBLE: + stateStr = _("inaccessible"); + break; + } + + /* Check if the pool is persistent or not */ + persistent = virStoragePoolIsPersistent(pool); + vshDebug(ctl, 5, "Persistent flag value: %d\n", persistent); + if (persistent < 0) + persistentStr = _("unknown"); + else + persistentStr = persistent ? _("yes") : _("no"); + + /* Display all information for this pool */ + vshPrint(ctl, "%-20s %-10s %-10s %-11s", + virStoragePoolGetName(pool), + stateStr, + autostartStr, + persistentStr); + + /* Display the capacity related quantities */ + if (info.state == VIR_STORAGE_POOL_RUNNING || + info.state == VIR_STORAGE_POOL_DEGRADED) { + double val; + const char *unit; + virBuffer infoBufStr = VIR_BUFFER_INITIALIZER; + + val = prettyCapacity(info.capacity, &unit); + virBufferVSprintf(&infoBufStr, "%.2lf %s", val, unit); + vshPrint(ctl, " %-9s", virBufferContentAndReset(&infoBufStr)); + + val = prettyCapacity(info.allocation, &unit); + virBufferVSprintf(&infoBufStr, "%.2lf %s", val, unit); + vshPrint(ctl, " %-11s", virBufferContentAndReset(&infoBufStr)); + + val = prettyCapacity(info.available, &unit); + virBufferVSprintf(&infoBufStr, "%.2lf %s", val, unit); + vshPrint(ctl, " %-10s\n", virBufferContentAndReset(&infoBufStr)); + } else + vshPrint(ctl, " %-9s %-11s %-10s\n", "-", "-", "-"); + } else { + /* Display basic information pool information */ + vshPrint(ctl, "%-20s %-10s %-10s\n", + virStoragePoolGetName(pool), + _("active"), + autostartStr); + } + virStoragePoolFree(pool); VIR_FREE(activeNames[i]); } for (i = 0; i < maxinactive; i++) { virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, inactiveNames[i]); - const char *autostartStr; - int autostart = 0; + const char *autostartStr, *persistentStr; + int autostart = 0, persistent = 0; /* this kind of work with pools is not atomic operation */ if (!pool) { @@ -4979,10 +5060,29 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) else autostartStr = autostart ? _("yes") : _("no"); - vshPrint(ctl, "%-20s %-10s %-10s\n", - inactiveNames[i], - _("inactive"), - autostartStr); + if (details) { + /* Check if the pool is persistent or not */ + persistent = virStoragePoolIsPersistent(pool); + vshDebug(ctl, 5, "Persistent flag value: %d\n", persistent); + if (persistent < 0) + persistentStr = _("unknown"); + else + persistentStr = persistent ? _("yes") : _("no"); + + /* Display detailed pool information */ + vshPrint(ctl, "%-20s %-10s %-10s %-11s %-9s %-11s %-10s\n", + inactiveNames[i], + _("inactive"), + autostartStr, + persistentStr, + "-", "-", "-"); + } else { + /* Display basic pool information */ + vshPrint(ctl, "%-20s %-10s %-10s\n", + inactiveNames[i], + _("inactive"), + autostartStr); + } virStoragePoolFree(pool); VIR_FREE(inactiveNames[i]); diff --git a/tools/virsh.pod b/tools/virsh.pod index b1917ee..cec07e3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -732,11 +732,13 @@ variables, and defaults to C<vi>. Returns basic information about the I<pool> object. -=item B<pool-list> optional I<--inactive> I<--all> +=item B<pool-list> optional I<--inactive> I<--all> I<--details> List pool objects known to libvirt. By default, only pools in use by active domains are listed; I<--inactive> lists just the inactive -pools, and I<--all> lists all pools. +pools, and I<--all> lists all pools. The I<--details> option instructs +virsh to additionally display pool persistence and capacity related +information where available. =item B<pool-name> I<uuid> -- 1.7.0.1
On 06/18/2010 02:30 AM, 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
---
Output from the new option (hopefully this doesn't wrap):
virsh # pool-list --all --details Name State Autostart Persistent Capacity Allocation Available -------------------------------------------------------------------------------------- default running yes yes 1.79 TB 1.47 TB 326.02 GB image_dir running yes yes 1.79 TB 1.47 TB 326.02 GB tmp inactive no yes - - -
Didn't wrap for me, and yes, it looks nice.
@@ -4937,36 +4940,114 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter); } } - vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Display the appropriate heading */ + if (details) { + vshPrintExtra(ctl, "%-20s %-10s %-10s %-11s %-9s %-11s %-10s\n", + _("Name"), _("State"), _("Autostart"), _("Persistent"), + _("Capacity"), _("Allocation"), _("Available"));
If a translation of one of those headers is longer than the field width in your printf format string, it may lead to awkward formatting in other languages. But you're not the first instance of that issue, and it is not a show-stopper for this patch (rather, it points to something that we may want to clean up in the future).
- vshPrint(ctl, "%-20s %-10s %-10s\n", - virStoragePoolGetName(pool), - _("active"), - autostartStr); + /* If requested, collect the extended information for this pool */ + if (details) { + if (virStoragePoolGetInfo(pool, &info) != 0) { + vshError(ctl, "%s", _("Could not retrieve pool information")); + VIR_FREE(activeNames[i]); + continue;
Indentation is wonky here (3-5-4 instead of 4-4-4). ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/19/2010 02:23 AM, Eric Blake wrote: <snip>
If a translation of one of those headers is longer than the field width in your printf format string, it may lead to awkward formatting in other languages. But you're not the first instance of that issue, and it is not a show-stopper for this patch (rather, it points to something that we may want to clean up in the future).
Agreed. The vol-list patch shouldn't exhibit this problem though, as I pretty much rewrote it not to. After you've taken a look at the vol-list patch, if you reckon the approach there is ok then I can redo this patch in the same way fairly easily. Probably better to get this solved from the outset rather than adding more stuff to adjust later. ? -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On 06/18/2010 10:29 AM, Justin Clift wrote:
On 06/19/2010 02:23 AM, Eric Blake wrote: <snip>
If a translation of one of those headers is longer than the field width in your printf format string, it may lead to awkward formatting in other languages. But you're not the first instance of that issue, and it is not a show-stopper for this patch (rather, it points to something that we may want to clean up in the future).
Agreed. The vol-list patch shouldn't exhibit this problem though, as I pretty much rewrote it not to.
After you've taken a look at the vol-list patch, if you reckon the approach there is ok then I can redo this patch in the same way fairly easily. Probably better to get this solved from the outset rather than adding more stuff to adjust later.
Your call. I was about to push 1/2 after making the minor tweaks, but then I saw this email, so I'm leaving the decision up to you whether you'd rather have pool-list touched twice (add --details, then add smart-width) or just once (rewrite to do smart-width of --details from the start). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/19/2010 02:59 AM, Eric Blake wrote:
Your call. I was about to push 1/2 after making the minor tweaks, but then I saw this email, so I'm leaving the decision up to you whether you'd rather have pool-list touched twice (add --details, then add smart-width) or just once (rewrite to do smart-width of --details from the start).
No worries. Probably best for me to update it first, before you push it. It'll be a cleaner solution if we know the code's decent (and probably a good habit to keep me in). ;) Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
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 --- Submitting this patch for review by itself, rather than with the pool-list one as well, because things suddenly got very busy here. It'll be a few more days until I'm happy with the vol-list one and submit that. This patch is a complete rewrite from the previous version. This new version sizes each column to the longest string contained. Additionally, if the --details option is not given, the output string uses the same fixed width format and output from previous versions. Example output: virsh # pool-list Name State Autostart ----------------------------------------- default active yes image_dir active yes virsh # pool-list --all Name State Autostart ----------------------------------------- default active yes image_dir active yes tmp inactive no virsh # pool-list --details Name State Autostart Persistent Capacity Allocation Available -------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - - virsh # tools/virsh.c | 426 ++++++++++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 6 +- 2 files changed, 365 insertions(+), 67 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 0bf7443..60ca9ad 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4891,114 +4891,410 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; static int cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { - int inactive = vshCommandOptBool(cmd, "inactive"); + virStoragePoolInfo info; + char **poolNames = NULL; + int i, function_ret, ret; + int numActivePools = 0, numInactivePools = 0, numAllPools = 0; + size_t stringLength = 0, nameStrLength = 0; + size_t autostartStrLength = 0, persistStrLength = 0; + size_t stateStrLength = 0, capStrLength = 0; + size_t allocStrLength = 0, availStrLength = 0; + struct poolInfoText { + char *state; + char *autostart; + char *persistent; + char *capacity; + char *allocation; + char *available; + }; + struct poolInfoText **poolInfoTexts = NULL; + + /* Determine the options passed by the user */ int all = vshCommandOptBool(cmd, "all"); + int details = vshCommandOptBool(cmd, "details"); + int inactive = vshCommandOptBool(cmd, "inactive"); int active = !inactive || all ? 1 : 0; - int maxactive = 0, maxinactive = 0, i; - char **activeNames = NULL, **inactiveNames = NULL; inactive |= all; + /* Check the connection to libvirtd daemon is still working */ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + /* Retrieve the number of active storage pools */ if (active) { - maxactive = virConnectNumOfStoragePools(ctl->conn); - if (maxactive < 0) { + numActivePools = virConnectNumOfStoragePools(ctl->conn); + if (numActivePools < 0) { vshError(ctl, "%s", _("Failed to list active pools")); return FALSE; } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); - - if ((maxactive = virConnectListStoragePools(ctl->conn, activeNames, - maxactive)) < 0) { - vshError(ctl, "%s", _("Failed to list active pools")); - VIR_FREE(activeNames); - return FALSE; - } - - qsort(&activeNames[0], maxactive, sizeof(char *), namesorter); - } } + + /* Retrieve the number of inactive storage pools */ if (inactive) { - maxinactive = virConnectNumOfDefinedStoragePools(ctl->conn); - if (maxinactive < 0) { + numInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn); + if (numInactivePools < 0) { vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(activeNames); return FALSE; } - if (maxinactive) { - inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive); + } - if ((maxinactive = virConnectListDefinedStoragePools(ctl->conn, inactiveNames, maxinactive)) < 0) { - vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return FALSE; - } + /* 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; + } - qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter); + /* Allocate memory for arrays of storage pool names and info */ + poolNames = vshCalloc(ctl, numAllPools, sizeof(char *)); + poolInfoTexts = + vshCalloc(ctl, numAllPools, sizeof(struct poolInfoText *)); + + /* Retrieve a list of active storage pool names */ + if (active) { + if ((virConnectListStoragePools(ctl->conn, + poolNames, numActivePools)) < 0) { + vshError(ctl, "%s", _("Failed to list active pools")); + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + return FALSE; } } - vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); - for (i = 0; i < maxactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, activeNames[i]); - const char *autostartStr; - int autostart = 0; + /* Add the inactive storage pools to the end of the name list */ + if (inactive) { + if ((virConnectListDefinedStoragePools(ctl->conn, + &poolNames[numActivePools], + numInactivePools)) < 0) { + vshError(ctl, "%s", _("Failed to list inactive pools")); + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + return FALSE; + } + } + + /* Sort the storage pool names */ + qsort(poolNames, numAllPools, sizeof(char *), namesorter); + + /* Collect the storage pool information for display */ + for (i = 0; i < numAllPools; i++) { + int autostart = 0, persistent = 0; - /* this kind of work with pools is not atomic operation */ + /* Retrieve a pool object, looking it up by name */ + virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, + poolNames[i]); if (!pool) { - VIR_FREE(activeNames[i]); + VIR_FREE(poolNames[i]); continue; } + /* Allocate and clear memory for one row of pool info */ + poolInfoTexts[i] = vshCalloc(ctl, 1, sizeof(struct poolInfoText)); + + /* Retrieve the autostart status of the pool */ if (virStoragePoolGetAutostart(pool, &autostart) < 0) - autostartStr = _("no autostart"); + poolInfoTexts[i]->autostart = vshStrdup(ctl, _("no autostart")); else - autostartStr = autostart ? _("yes") : _("no"); + poolInfoTexts[i]->autostart = vshStrdup(ctl, autostart ? + _("yes") : _("no")); - vshPrint(ctl, "%-20s %-10s %-10s\n", - virStoragePoolGetName(pool), - _("active"), - autostartStr); + /* Retrieve the persistence status of the pool */ + persistent = virStoragePoolIsPersistent(pool); + vshDebug(ctl, 5, "Persistent flag value: %d\n", persistent); + if (persistent < 0) + poolInfoTexts[i]->persistent = vshStrdup(ctl, _("unknown")); + else + poolInfoTexts[i]->persistent = vshStrdup(ctl, persistent ? + _("yes") : _("no")); + + /* 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")); + } else { + /* Decide which state string to display */ + if (details) { + /* --details option was specified, we're using detailed state + * strings */ + switch (info.state) { + case VIR_STORAGE_POOL_INACTIVE: + poolInfoTexts[i]->state = vshStrdup(ctl, _("inactive")); + break; + case VIR_STORAGE_POOL_BUILDING: + poolInfoTexts[i]->state = vshStrdup(ctl, _("building")); + break; + case VIR_STORAGE_POOL_RUNNING: + poolInfoTexts[i]->state = vshStrdup(ctl, _("running")); + break; + case VIR_STORAGE_POOL_DEGRADED: + poolInfoTexts[i]->state = vshStrdup(ctl, _("degraded")); + break; + case VIR_STORAGE_POOL_INACCESSIBLE: + poolInfoTexts[i]->state = vshStrdup(ctl, _("inaccessible")); + break; + } + + /* Create the pool size related strings */ + if (info.state == VIR_STORAGE_POOL_RUNNING || + info.state == VIR_STORAGE_POOL_DEGRADED) { + double val; + const char *unit; + + /* Create the capacity output string */ + val = prettyCapacity(info.capacity, &unit); + ret = virAsprintf(&poolInfoTexts[i]->capacity, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + + /* Create the allocation output string */ + val = prettyCapacity(info.allocation, &unit); + ret = virAsprintf(&poolInfoTexts[i]->allocation, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + + /* Create the available space output string */ + val = prettyCapacity(info.available, &unit); + ret = virAsprintf(&poolInfoTexts[i]->available, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + } else { + /* Capacity related information isn't available */ + poolInfoTexts[i]->capacity = vshStrdup(ctl, _("-")); + poolInfoTexts[i]->allocation = vshStrdup(ctl, _("-")); + poolInfoTexts[i]->available = vshStrdup(ctl, _("-")); + } + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->capacity); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->allocation); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Keep the length of available string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->available); + if (stringLength > availStrLength) + availStrLength = stringLength; + + /* Keep the length of persistent string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->persistent); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + } else { + /* --details option was not specified, only active/inactive + * state strings are used */ + if (info.state == VIR_STORAGE_POOL_INACTIVE) + poolInfoTexts[i]->state = vshStrdup(ctl, _("inactive")); + else + poolInfoTexts[i]->state = vshStrdup(ctl, _("active")); + } + } + + /* Keep the length of name string if longest so far */ + stringLength = strlen(poolNames[i]); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of state string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->state); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Keep the length of autostart string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->autostart); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Free the pool object */ virStoragePoolFree(pool); - VIR_FREE(activeNames[i]); } - for (i = 0; i < maxinactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, inactiveNames[i]); - const char *autostartStr; - int autostart = 0; - /* this kind of work with pools is not atomic operation */ - if (!pool) { - VIR_FREE(inactiveNames[i]); - continue; + /* If the --details option wasn't selected, we output the pool + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* Output old style header */ + vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), + _("Autostart")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Output old style pool info */ + for (i = 0; i < numAllPools; i++) { + vshPrint(ctl, "%-20s %-10s %-10s\n", + poolNames[i], + poolInfoTexts[i]->state, + poolInfoTexts[i]->autostart); } - if (virStoragePoolGetAutostart(pool, &autostart) < 0) - autostartStr = _("no autostart"); - else - autostartStr = autostart ? _("yes") : _("no"); + /* Cleanup and return */ + function_ret = TRUE; + goto cleanup; + } - vshPrint(ctl, "%-20s %-10s %-10s\n", - inactiveNames[i], - _("inactive"), - autostartStr); + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Use the length of state header string if it's longest */ + stringLength = strlen(_("State")); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Use the length of autostart header string if it's longest */ + stringLength = strlen(_("Autostart")); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Use the length of persistent header string if it's longest */ + stringLength = strlen(_("Persistent")); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Use the length of available header string if it's longest */ + stringLength = strlen(_("Available")); + if (stringLength > availStrLength) + availStrLength = stringLength; + + /* Display the string lengths for debugging. */ + vshDebug(ctl, 5, "Longest name string = %lu chars\n", + (unsigned long) nameStrLength); + vshDebug(ctl, 5, "Longest state string = %lu chars\n", + (unsigned long) stateStrLength); + vshDebug(ctl, 5, "Longest autostart string = %lu chars\n", + (unsigned long) autostartStrLength); + vshDebug(ctl, 5, "Longest persistent string = %lu chars\n", + (unsigned long) persistStrLength); + vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", + (unsigned long) capStrLength); + vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", + (unsigned long) allocStrLength); + vshDebug(ctl, 5, "Longest available string = %lu chars\n", + (unsigned long) availStrLength); + + /* Create the output template. Each column is sized according to + * the longest string. + */ + char *outputStr; + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%-%lus %%%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) stateStrLength, + (unsigned long) autostartStrLength, + (unsigned long) persistStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength, + (unsigned long) availStrLength); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("State"), _("Autostart"), + _("Persistent"), ("Capacity"), _("Allocation"), _("Available")); + for (i = nameStrLength + stateStrLength + autostartStrLength + + persistStrLength + capStrLength + + allocStrLength + availStrLength + + 12; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the pool info rows */ + for (i = 0; i < numAllPools; i++) { + vshPrint(ctl, outputStr, + poolNames[i], + poolInfoTexts[i]->state, + poolInfoTexts[i]->autostart, + poolInfoTexts[i]->persistent, + poolInfoTexts[i]->capacity, + poolInfoTexts[i]->allocation, + poolInfoTexts[i]->available); + } + + /* Cleanup and return */ + function_ret = TRUE; + goto cleanup; - virStoragePoolFree(pool); - VIR_FREE(inactiveNames[i]); +asprintf_failure: + + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); } - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return TRUE; + +cleanup: + + /* Safely free the memory allocated in this function */ + for (i = 0; i < numAllPools; i++) { + /* Cleanup the memory for one pool info structure */ + if (poolInfoTexts[i] != NULL) { + VIR_FREE(poolInfoTexts[i]->state); + VIR_FREE(poolInfoTexts[i]->autostart); + VIR_FREE(poolInfoTexts[i]->persistent); + VIR_FREE(poolInfoTexts[i]->capacity); + VIR_FREE(poolInfoTexts[i]->allocation); + VIR_FREE(poolInfoTexts[i]->available); + VIR_FREE(poolInfoTexts[i]); + } + VIR_FREE(poolNames[i]); + } + + /* Cleanup the memory for the initial arrays*/ + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + + /* Return the desired value */ + return function_ret; } /* diff --git a/tools/virsh.pod b/tools/virsh.pod index 05ba731..7c75edc 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -732,11 +732,13 @@ variables, and defaults to C<vi>. Returns basic information about the I<pool> object. -=item B<pool-list> optional I<--inactive> I<--all> +=item B<pool-list> optional I<--inactive> I<--all> I<--details> List pool objects known to libvirt. By default, only pools in use by active domains are listed; I<--inactive> lists just the inactive -pools, and I<--all> lists all pools. +pools, and I<--all> lists all pools. The I<--details> option instructs +virsh to additionally display pool persistence and capacity related +information where available. =item B<pool-name> I<uuid> -- 1.7.0.1
Submitting this patch for review by itself, rather than with the
On 06/21/2010 11:45 PM, Justin Clift wrote: <snip> pool-list
one as well ...
Ugh, typo. I meant to say I'm submitting this one now for review, rather than with the matching *vol-list* one. :) Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
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 --- (This is the v3 patch version, but with some wonky spacing fixed.) Submitting this patch for review by itself, rather than with the pool-list one as well, because things suddenly got very busy here. It'll be a few more days until I'm happy with the vol-list one and submit that. This patch is a complete rewrite from the previous version. This new version sizes each column to the longest string contained. Additionally, if the --details option is not given, the output string uses the same fixed width format and output from previous versions. Example output: virsh # pool-list Name State Autostart ----------------------------------------- default active yes image_dir active yes virsh # pool-list --all Name State Autostart ----------------------------------------- default active yes image_dir active yes tmp inactive no virsh # pool-list --details Name State Autostart Persistent Capacity Allocation Available -------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - - virsh # tools/virsh.c | 426 ++++++++++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 6 +- 2 files changed, 365 insertions(+), 67 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 0bf7443..a825495 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4891,114 +4891,410 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; static int cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { - int inactive = vshCommandOptBool(cmd, "inactive"); + virStoragePoolInfo info; + char **poolNames = NULL; + int i, function_ret, ret; + int numActivePools = 0, numInactivePools = 0, numAllPools = 0; + size_t stringLength = 0, nameStrLength = 0; + size_t autostartStrLength = 0, persistStrLength = 0; + size_t stateStrLength = 0, capStrLength = 0; + size_t allocStrLength = 0, availStrLength = 0; + struct poolInfoText { + char *state; + char *autostart; + char *persistent; + char *capacity; + char *allocation; + char *available; + }; + struct poolInfoText **poolInfoTexts = NULL; + + /* Determine the options passed by the user */ int all = vshCommandOptBool(cmd, "all"); + int details = vshCommandOptBool(cmd, "details"); + int inactive = vshCommandOptBool(cmd, "inactive"); int active = !inactive || all ? 1 : 0; - int maxactive = 0, maxinactive = 0, i; - char **activeNames = NULL, **inactiveNames = NULL; inactive |= all; + /* Check the connection to libvirtd daemon is still working */ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + /* Retrieve the number of active storage pools */ if (active) { - maxactive = virConnectNumOfStoragePools(ctl->conn); - if (maxactive < 0) { + numActivePools = virConnectNumOfStoragePools(ctl->conn); + if (numActivePools < 0) { vshError(ctl, "%s", _("Failed to list active pools")); return FALSE; } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); - - if ((maxactive = virConnectListStoragePools(ctl->conn, activeNames, - maxactive)) < 0) { - vshError(ctl, "%s", _("Failed to list active pools")); - VIR_FREE(activeNames); - return FALSE; - } - - qsort(&activeNames[0], maxactive, sizeof(char *), namesorter); - } } + + /* Retrieve the number of inactive storage pools */ if (inactive) { - maxinactive = virConnectNumOfDefinedStoragePools(ctl->conn); - if (maxinactive < 0) { + numInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn); + if (numInactivePools < 0) { vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(activeNames); return FALSE; } - if (maxinactive) { - inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive); + } - if ((maxinactive = virConnectListDefinedStoragePools(ctl->conn, inactiveNames, maxinactive)) < 0) { - vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return FALSE; - } + /* 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; + } - qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter); + /* Allocate memory for arrays of storage pool names and info */ + poolNames = vshCalloc(ctl, numAllPools, sizeof(char *)); + poolInfoTexts = + vshCalloc(ctl, numAllPools, sizeof(struct poolInfoText *)); + + /* Retrieve a list of active storage pool names */ + if (active) { + if ((virConnectListStoragePools(ctl->conn, + poolNames, numActivePools)) < 0) { + vshError(ctl, "%s", _("Failed to list active pools")); + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + return FALSE; } } - vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); - for (i = 0; i < maxactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, activeNames[i]); - const char *autostartStr; - int autostart = 0; + /* Add the inactive storage pools to the end of the name list */ + if (inactive) { + if ((virConnectListDefinedStoragePools(ctl->conn, + &poolNames[numActivePools], + numInactivePools)) < 0) { + vshError(ctl, "%s", _("Failed to list inactive pools")); + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + return FALSE; + } + } + + /* Sort the storage pool names */ + qsort(poolNames, numAllPools, sizeof(char *), namesorter); - /* this kind of work with pools is not atomic operation */ + /* Collect the storage pool information for display */ + for (i = 0; i < numAllPools; i++) { + int autostart = 0, persistent = 0; + + /* Retrieve a pool object, looking it up by name */ + virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, + poolNames[i]); if (!pool) { - VIR_FREE(activeNames[i]); + VIR_FREE(poolNames[i]); continue; } + /* Allocate and clear memory for one row of pool info */ + poolInfoTexts[i] = vshCalloc(ctl, 1, sizeof(struct poolInfoText)); + + /* Retrieve the autostart status of the pool */ if (virStoragePoolGetAutostart(pool, &autostart) < 0) - autostartStr = _("no autostart"); + poolInfoTexts[i]->autostart = vshStrdup(ctl, _("no autostart")); else - autostartStr = autostart ? _("yes") : _("no"); + poolInfoTexts[i]->autostart = vshStrdup(ctl, autostart ? + _("yes") : _("no")); - vshPrint(ctl, "%-20s %-10s %-10s\n", - virStoragePoolGetName(pool), - _("active"), - autostartStr); + /* Retrieve the persistence status of the pool */ + persistent = virStoragePoolIsPersistent(pool); + vshDebug(ctl, 5, "Persistent flag value: %d\n", persistent); + if (persistent < 0) + poolInfoTexts[i]->persistent = vshStrdup(ctl, _("unknown")); + else + poolInfoTexts[i]->persistent = vshStrdup(ctl, persistent ? + _("yes") : _("no")); + + /* 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")); + } else { + /* Decide which state string to display */ + if (details) { + /* --details option was specified, we're using detailed state + * strings */ + switch (info.state) { + case VIR_STORAGE_POOL_INACTIVE: + poolInfoTexts[i]->state = vshStrdup(ctl, _("inactive")); + break; + case VIR_STORAGE_POOL_BUILDING: + poolInfoTexts[i]->state = vshStrdup(ctl, _("building")); + break; + case VIR_STORAGE_POOL_RUNNING: + poolInfoTexts[i]->state = vshStrdup(ctl, _("running")); + break; + case VIR_STORAGE_POOL_DEGRADED: + poolInfoTexts[i]->state = vshStrdup(ctl, _("degraded")); + break; + case VIR_STORAGE_POOL_INACCESSIBLE: + poolInfoTexts[i]->state = vshStrdup(ctl, _("inaccessible")); + break; + } + + /* Create the pool size related strings */ + if (info.state == VIR_STORAGE_POOL_RUNNING || + info.state == VIR_STORAGE_POOL_DEGRADED) { + double val; + const char *unit; + + /* Create the capacity output string */ + val = prettyCapacity(info.capacity, &unit); + ret = virAsprintf(&poolInfoTexts[i]->capacity, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + + /* Create the allocation output string */ + val = prettyCapacity(info.allocation, &unit); + ret = virAsprintf(&poolInfoTexts[i]->allocation, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + + /* Create the available space output string */ + val = prettyCapacity(info.available, &unit); + ret = virAsprintf(&poolInfoTexts[i]->available, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + } else { + /* Capacity related information isn't available */ + poolInfoTexts[i]->capacity = vshStrdup(ctl, _("-")); + poolInfoTexts[i]->allocation = vshStrdup(ctl, _("-")); + poolInfoTexts[i]->available = vshStrdup(ctl, _("-")); + } + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->capacity); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->allocation); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Keep the length of available string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->available); + if (stringLength > availStrLength) + availStrLength = stringLength; + + /* Keep the length of persistent string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->persistent); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + } else { + /* --details option was not specified, only active/inactive + * state strings are used */ + if (info.state == VIR_STORAGE_POOL_INACTIVE) + poolInfoTexts[i]->state = vshStrdup(ctl, _("inactive")); + else + poolInfoTexts[i]->state = vshStrdup(ctl, _("active")); + } + } + + /* Keep the length of name string if longest so far */ + stringLength = strlen(poolNames[i]); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of state string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->state); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Keep the length of autostart string if longest so far */ + stringLength = strlen(poolInfoTexts[i]->autostart); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Free the pool object */ virStoragePoolFree(pool); - VIR_FREE(activeNames[i]); } - for (i = 0; i < maxinactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, inactiveNames[i]); - const char *autostartStr; - int autostart = 0; - /* this kind of work with pools is not atomic operation */ - if (!pool) { - VIR_FREE(inactiveNames[i]); - continue; + /* If the --details option wasn't selected, we output the pool + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* Output old style header */ + vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), + _("Autostart")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Output old style pool info */ + for (i = 0; i < numAllPools; i++) { + vshPrint(ctl, "%-20s %-10s %-10s\n", + poolNames[i], + poolInfoTexts[i]->state, + poolInfoTexts[i]->autostart); } - if (virStoragePoolGetAutostart(pool, &autostart) < 0) - autostartStr = _("no autostart"); - else - autostartStr = autostart ? _("yes") : _("no"); + /* Cleanup and return */ + function_ret = TRUE; + goto cleanup; + } - vshPrint(ctl, "%-20s %-10s %-10s\n", - inactiveNames[i], - _("inactive"), - autostartStr); + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Use the length of state header string if it's longest */ + stringLength = strlen(_("State")); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Use the length of autostart header string if it's longest */ + stringLength = strlen(_("Autostart")); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Use the length of persistent header string if it's longest */ + stringLength = strlen(_("Persistent")); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Use the length of available header string if it's longest */ + stringLength = strlen(_("Available")); + if (stringLength > availStrLength) + availStrLength = stringLength; + + /* Display the string lengths for debugging. */ + vshDebug(ctl, 5, "Longest name string = %lu chars\n", + (unsigned long) nameStrLength); + vshDebug(ctl, 5, "Longest state string = %lu chars\n", + (unsigned long) stateStrLength); + vshDebug(ctl, 5, "Longest autostart string = %lu chars\n", + (unsigned long) autostartStrLength); + vshDebug(ctl, 5, "Longest persistent string = %lu chars\n", + (unsigned long) persistStrLength); + vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", + (unsigned long) capStrLength); + vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", + (unsigned long) allocStrLength); + vshDebug(ctl, 5, "Longest available string = %lu chars\n", + (unsigned long) availStrLength); + + /* Create the output template. Each column is sized according to + * the longest string. + */ + char *outputStr; + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%-%lus %%%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) stateStrLength, + (unsigned long) autostartStrLength, + (unsigned long) persistStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength, + (unsigned long) availStrLength); + if (ret < 0) { + /* An error occurred creating the string, return */ + function_ret = FALSE; + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("State"), _("Autostart"), + _("Persistent"), ("Capacity"), _("Allocation"), _("Available")); + for (i = nameStrLength + stateStrLength + autostartStrLength + + persistStrLength + capStrLength + + allocStrLength + availStrLength + + 12; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the pool info rows */ + for (i = 0; i < numAllPools; i++) { + vshPrint(ctl, outputStr, + poolNames[i], + poolInfoTexts[i]->state, + poolInfoTexts[i]->autostart, + poolInfoTexts[i]->persistent, + poolInfoTexts[i]->capacity, + poolInfoTexts[i]->allocation, + poolInfoTexts[i]->available); + } + + /* Cleanup and return */ + function_ret = TRUE; + goto cleanup; - virStoragePoolFree(pool); - VIR_FREE(inactiveNames[i]); +asprintf_failure: + + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); } - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return TRUE; + +cleanup: + + /* Safely free the memory allocated in this function */ + for (i = 0; i < numAllPools; i++) { + /* Cleanup the memory for one pool info structure */ + if (poolInfoTexts[i] != NULL) { + VIR_FREE(poolInfoTexts[i]->state); + VIR_FREE(poolInfoTexts[i]->autostart); + VIR_FREE(poolInfoTexts[i]->persistent); + VIR_FREE(poolInfoTexts[i]->capacity); + VIR_FREE(poolInfoTexts[i]->allocation); + VIR_FREE(poolInfoTexts[i]->available); + VIR_FREE(poolInfoTexts[i]); + } + VIR_FREE(poolNames[i]); + } + + /* Cleanup the memory for the initial arrays*/ + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + + /* Return the desired value */ + return function_ret; } /* diff --git a/tools/virsh.pod b/tools/virsh.pod index 05ba731..7c75edc 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -732,11 +732,13 @@ variables, and defaults to C<vi>. Returns basic information about the I<pool> object. -=item B<pool-list> optional I<--inactive> I<--all> +=item B<pool-list> optional I<--inactive> I<--all> I<--details> List pool objects known to libvirt. By default, only pools in use by active domains are listed; I<--inactive> lists just the inactive -pools, and I<--all> lists all pools. +pools, and I<--all> lists all pools. The I<--details> option instructs +virsh to additionally display pool persistence and capacity related +information where available. =item B<pool-name> I<uuid> -- 1.7.0.1
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jun 22, 2010 at 11:44:57AM -0600, Eric Blake wrote:
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?
Correct, we shouldn't change this behaviour - it'll break apps parsing the output Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Jun 22, 2010 at 06:54:55PM +0100, Daniel P. Berrange wrote:
On Tue, Jun 22, 2010 at 11:44:57AM -0600, Eric Blake wrote:
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?
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it... --Hugh
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output? Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client): virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - - -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
--details is still targetted at humans. If you want something more easily parseable it should use a structured format like CSV. So I don't think we should be overloading --details for this purpose. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 06/23/2010 06:58 PM, Daniel P. Berrange wrote: <snip>
--details is still targetted at humans. If you want something more easily parseable it should use a structured format like CSV. So I don't think we should be overloading --details for this purpose.
It sounds like there would be good benefit to directly improving the output of virsh for scripting. As a thought for the right approach, maybe having command line options affecting the output delimiters (field & record separators), suppress headings, and using a "better approach" for errors? Initial thinking is something like: -D --data-only data only (no header in output) -F, --field-separator=STRING set field separator (default: "|") -R, --record-separator=STRING set record separator (default: newline) Unsure about error handling though. Any suggestions? Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On Wed, Jun 23, 2010 at 09:58:34AM +0100, Daniel P. Berrange wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
--details is still targetted at humans. If you want something more easily parseable it should use a structured format like CSV. So I don't think we should be overloading --details for this purpose.
CSV is a good format, but beware the many ways to shoot yourself in the foot. I recommend using my program "csvtool" (in Fedora/Debian) which can fully and safely parse CSV output from shell scripts, or use a library (eg. Text::CSV for Perl, or csv for Python). More about this subject here: http://libguestfs.org/virt-df.1.html#note_about_csv_format Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
On Thu, Jul 01, 2010 at 11:42:12AM +0100, Richard W.M. Jones wrote:
On Wed, Jun 23, 2010 at 09:58:34AM +0100, Daniel P. Berrange wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
--details is still targetted at humans. If you want something more easily parseable it should use a structured format like CSV. So I don't think we should be overloading --details for this purpose.
CSV is a good format, but beware the many ways to shoot yourself in the foot. I recommend using my program "csvtool" (in Fedora/Debian) which can fully and safely parse CSV output from shell scripts, or use a library (eg. Text::CSV for Perl, or csv for Python). More about this subject here:
http://libguestfs.org/virt-df.1.html#note_about_csv_format
Rich.
If we're going to go to all this trouble, how much more difficult would it be to implement something like ps -o and give the user control of the format? Dave
On Thu, Jul 01, 2010 at 10:51:00AM -0400, Dave Allan wrote:
On Thu, Jul 01, 2010 at 11:42:12AM +0100, Richard W.M. Jones wrote:
On Wed, Jun 23, 2010 at 09:58:34AM +0100, Daniel P. Berrange wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
--details is still targetted at humans. If you want something more easily parseable it should use a structured format like CSV. So I don't think we should be overloading --details for this purpose.
CSV is a good format, but beware the many ways to shoot yourself in the foot. I recommend using my program "csvtool" (in Fedora/Debian) which can fully and safely parse CSV output from shell scripts, or use a library (eg. Text::CSV for Perl, or csv for Python). More about this subject here:
http://libguestfs.org/virt-df.1.html#note_about_csv_format
Rich.
If we're going to go to all this trouble, how much more difficult would it be to implement something like ps -o and give the user control of the format?
I think the two issues are orthogonal. Having said that, csvtool lets you pull out columns by name, so virsh could change the order or add new columns in future without causing any (csvtool-using) scripts to break. In the example below, I pick columns from the output of 'virt-df --csv' to demonstrate how this works with these existing tools: # virt-df --csv | csvtool namedcol "Virtual Machine,Use%" - CentOS5x32,45.2% CentOS5x32,35.8% Debian5x64,35.6% Debian5x64,60.9% Debian5x64,3.3% Debian5x64,40.5% Debian5x64,15.2% Debian5x64,20.3% F13x64,6.0% F13x64,42.2% [...truncated...] Even if virt-df was to rearrange or add columns in future, this would continue to work. If a column was deleted from the output of virt-df, then you'd get an empty result column. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On Thu, Jul 01, 2010 at 04:06:36PM +0100, Richard W.M. Jones wrote:
If a column was deleted from the output of virt-df, then you'd get an empty result column.
Actually this is not true. csvtool gives an error: sudo virt-df --csv | csvtool namedcol "Virtual Machine,X" - Fatal error: exception Failure("namedcol: requested header not in CSV file: X") Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
On 07/01/2010 08:42 PM, Richard W.M. Jones wrote: <snip>
CSV is a good format, but beware the many ways to shoot yourself in the foot. I recommend using my program "csvtool" (in Fedora/Debian) which can fully and safely parse CSV output from shell scripts, or use a library (eg. Text::CSV for Perl, or csv for Python). More about this subject here:
Is the correct workaround here being able to specify the field and record delimiters? To (for example) use "|" or "\t" instead of ",". Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On Fri, Jul 02, 2010 at 12:59:36AM +1000, Justin Clift wrote:
On 07/01/2010 08:42 PM, Richard W.M. Jones wrote: <snip>
CSV is a good format, but beware the many ways to shoot yourself in the foot. I recommend using my program "csvtool" (in Fedora/Debian) which can fully and safely parse CSV output from shell scripts, or use a library (eg. Text::CSV for Perl, or csv for Python). More about this subject here:
Is the correct workaround here being able to specify the field and record delimiters? To (for example) use "|" or "\t" instead of ",".
The CSV format is rather underspecified as it is. csvtool was written based on what a certain popular spreadsheet can do, plus much folk wisdom. Changing the delimiter doesn't particularly help. Basically you have to be able to parse CSV files like this one from the csvtool test suite: ---------------------------------------------------------------------- "This is a test with commas,,,,, and carriage returns and "0",a second field,a third field a fourth field on a new line ---------------------------------------------------------------------- (That CSV file has two rows and three columns) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
On 07/02/2010 01:11 AM, Richard W.M. Jones wrote: <snip>
The CSV format is rather underspecified as it is. csvtool was written based on what a certain popular spreadsheet can do, plus much folk wisdom. Changing the delimiter doesn't particularly help.
It seems like a really useful utility, useful in lots of situations. Is it in RHEL (6) as well as Fedora? For the discussion here, kind of wondering what we can do to optimise the CSV output? We don't know which utilities the user will have on their system, and adding a hard dependency on csvtool may not be cool. ? Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On Fri, Jul 02, 2010 at 01:17:05AM +1000, Justin Clift wrote:
On 07/02/2010 01:11 AM, Richard W.M. Jones wrote: <snip>
The CSV format is rather underspecified as it is. csvtool was written based on what a certain popular spreadsheet can do, plus much folk wisdom. Changing the delimiter doesn't particularly help.
It seems like a really useful utility, useful in lots of situations. Is it in RHEL (6) as well as Fedora?
Yes, in RHEL 6 (not in RHEL 5, but it is in EPEL). And Fedora and Debian.
For the discussion here, kind of wondering what we can do to optimise the CSV output? We don't know which utilities the user will have on their system, and adding a hard dependency on csvtool may not be cool.
If you go down the CSV output path, then you have to accept that some people may believe they can do: virsh --csv ... | awk -F, '{ print $2 }' and that this is going to Fail in a Bad Way for them at some point. Furthermore they might not understand why it's bad, and may even disagree with you on this. What we did in the other virt-* tools that can generate CSV output is to add very large warnings to the man pages and other documentation which points to the right tools to use (not just csvtool, but using the CSV libraries in your language of choice). Whitespace-separated output, the original topic of this thread, is still easier to parse from shell scripts given that you don't/can't use any external tool. Provided the fields don't contain any internal whitespace and are never empty. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client):
Sorry, been away for a couple of weeks.
virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - -
One good thing, and several bad things about that. The good thing is that empty columns are presented with '-' which means you can use awk and sort -k to parse the output columnwise. The bad things: * Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work). * Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers. * Unnecessary "-------" line. * Title line should be optional. Have a --no-title option or something like that to suppress it. * Does virsh still print an unnecessary blank line after the output? If so, stop doing that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On Thu, Jul 01, 2010 at 11:38:06AM +0100, Richard W.M. Jones wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client):
Sorry, been away for a couple of weeks.
virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - -
One good thing, and several bad things about that. The good thing is that empty columns are presented with '-' which means you can use awk and sort -k to parse the output columnwise.
The bad things:
* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
* Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers.
* Unnecessary "-------" line.
* Title line should be optional. Have a --no-title option or something like that to suppress it.
* Does virsh still print an unnecessary blank line after the output? If so, stop doing that.
All of these bad things are just an artifact of trying to use the same output format for humans & machines. To address all those would make the result unpleasant for humans. We really do just need to create a dedicated format for machines, csv or json, or somethingelse and leave the human format alone Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jul 01, 2010 at 11:50:20AM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 11:38:06AM +0100, Richard W.M. Jones wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client):
Sorry, been away for a couple of weeks.
virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - -
One good thing, and several bad things about that. The good thing is that empty columns are presented with '-' which means you can use awk and sort -k to parse the output columnwise.
The bad things:
* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
* Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers.
* Unnecessary "-------" line.
* Title line should be optional. Have a --no-title option or something like that to suppress it.
* Does virsh still print an unnecessary blank line after the output? If so, stop doing that.
All of these bad things are just an artifact of trying to use the same output format for humans & machines. To address all those would make the result unpleasant for humans. We really do just need to create a dedicated format for machines, csv or json, or somethingelse and leave the human format alone
Actually I think we *do* need the --details convenience method for human-readable output, *and* a better machine-parseable format. Justin is mostly interested in making virsh more usable for people, which I fully support -- making it easier to script is also a bonus however. --Hugh
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 01, 2010 at 08:57:25AM -0400, Hugh O. Brock wrote:
On Thu, Jul 01, 2010 at 11:50:20AM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 11:38:06AM +0100, Richard W.M. Jones wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client):
Sorry, been away for a couple of weeks.
virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - -
One good thing, and several bad things about that. The good thing is that empty columns are presented with '-' which means you can use awk and sort -k to parse the output columnwise.
The bad things:
* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
* Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers.
* Unnecessary "-------" line.
* Title line should be optional. Have a --no-title option or something like that to suppress it.
* Does virsh still print an unnecessary blank line after the output? If so, stop doing that.
All of these bad things are just an artifact of trying to use the same output format for humans & machines. To address all those would make the result unpleasant for humans. We really do just need to create a dedicated format for machines, csv or json, or somethingelse and leave the human format alone
Actually I think we *do* need the --details convenience method for human-readable output, *and* a better machine-parseable format. Justin is mostly interested in making virsh more usable for people, which I fully support -- making it easier to script is also a bonus however.
Err, that's entirely my point Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jul 01, 2010 at 02:01:57PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 08:57:25AM -0400, Hugh O. Brock wrote:
On Thu, Jul 01, 2010 at 11:50:20AM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 11:38:06AM +0100, Richard W.M. Jones wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
> Correct, we shouldn't change this behaviour - it'll break apps parsing > the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client):
Sorry, been away for a couple of weeks.
virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - -
One good thing, and several bad things about that. The good thing is that empty columns are presented with '-' which means you can use awk and sort -k to parse the output columnwise.
The bad things:
* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
* Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers.
* Unnecessary "-------" line.
* Title line should be optional. Have a --no-title option or something like that to suppress it.
* Does virsh still print an unnecessary blank line after the output? If so, stop doing that.
All of these bad things are just an artifact of trying to use the same output format for humans & machines. To address all those would make the result unpleasant for humans. We really do just need to create a dedicated format for machines, csv or json, or somethingelse and leave the human format alone
Actually I think we *do* need the --details convenience method for human-readable output, *and* a better machine-parseable format. Justin is mostly interested in making virsh more usable for people, which I fully support -- making it easier to script is also a bonus however.
Err, that's entirely my point
Oh, excellent! :) --Hugh
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 01, 2010 at 02:01:57PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 08:57:25AM -0400, Hugh O. Brock wrote:
On Thu, Jul 01, 2010 at 11:50:20AM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 11:38:06AM +0100, Richard W.M. Jones wrote:
On Tue, Jun 22, 2010 at 12:35:32PM -0600, Eric Blake wrote:
On 06/22/2010 12:24 PM, Hugh O. Brock wrote:
> Correct, we shouldn't change this behaviour - it'll break apps parsing > the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
The new --details option is our chance to change output - it outputs whatever format we want, because it is a new flag; Rich, do you have any preferences about what it _should_ output?
Here's what pool-list --details would currently do, if we applied Justin's patch as-is (modulo no line wrapping added by my email client):
Sorry, been away for a couple of weeks.
virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - -
One good thing, and several bad things about that. The good thing is that empty columns are presented with '-' which means you can use awk and sort -k to parse the output columnwise.
The bad things:
* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
* Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers.
* Unnecessary "-------" line.
* Title line should be optional. Have a --no-title option or something like that to suppress it.
* Does virsh still print an unnecessary blank line after the output? If so, stop doing that.
All of these bad things are just an artifact of trying to use the same output format for humans & machines. To address all those would make the result unpleasant for humans. We really do just need to create a dedicated format for machines, csv or json, or somethingelse and leave the human format alone
Actually I think we *do* need the --details convenience method for human-readable output, *and* a better machine-parseable format. Justin is mostly interested in making virsh more usable for people, which I fully support -- making it easier to script is also a bonus however.
Err, that's entirely my point
I'm not looking at this too closely, but isn't this discussion about a potential patch (adding --details)? If so then if it costs little to make it machine friendly by default, why not just do it. Changing existing output is a little difficult though. I'm concerned about having two code paths (normal vs machine readable), with the implication that the machine readable path will bit rot because it gets less visible use. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On Thu, Jul 01, 2010 at 02:44:35PM +0100, Richard W.M. Jones wrote:
On Thu, Jul 01, 2010 at 02:01:57PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 08:57:25AM -0400, Hugh O. Brock wrote:
On Thu, Jul 01, 2010 at 11:50:20AM +0100, Daniel P. Berrange wrote:
On Thu, Jul 01, 2010 at 11:38:06AM +0100, Richard W.M. Jones wrote:
The bad things:
* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
* Numeric fields aren't numbers: You can't sort -n on "1.79 TB", and you can't read that number into a script and do math on it. Most tools have a "-h" or "--human" option in order to generate human- readable numbers (without spaces), but default to just printing the raw numbers.
* Unnecessary "-------" line.
* Title line should be optional. Have a --no-title option or something like that to suppress it.
* Does virsh still print an unnecessary blank line after the output? If so, stop doing that.
[snip]
I'm not looking at this too closely, but isn't this discussion about a potential patch (adding --details)? If so then if it costs little to make it machine friendly by default, why not just do it.
The changes noted above to make it machine friendly, make it very unfriendly to humans. The patch was focused on making this more friendly for humans.
I'm concerned about having two code paths (normal vs machine readable), with the implication that the machine readable path will bit rot because it gets less visible use.
virsh data output is easily tested using the test:///default drive - we already have a bunch of test cases for several virsh commands. If we did add machine readable code though, I agree we want to avoid having two parallel code paths. The current code is rather mixing up the jobs of fetching the data from libvirt with the job of printing the data out. I'd say we should separate this logic out, so the presentation code is completely isolated & sharable across commands. We have 3 data formats commonly used in human friendly output a. Table format # virsh list Id Name State ---------------------------------- - f12i686 shut off - f12i686_v1 shut off - f13i686 shut off - f13x86_64 shut off b. key value pair format # virsh dominfo f12i686 Id: - Name: f12i686 UUID: 0301f2c1-05fc-dddf-089b-60e874f11e64 OS Type: hvm State: shut off CPU(s): 1 Max memory: 1048576 kB Used memory: 1048576 kB Persistent: yes c. single value # virsh domuuid f12i686 0301f2c1-05fc-dddf-089b-60e874f11e64 In terms of data structures, we could represent the first using an array of hashes, the second using a single hash, and the third using a string. We can then build re-usable data formatters for each of these data structures. virshPrintString(const char * value) virshPrintHash(const char ** headings, virHashPtr data) virshPrintHashList(const char ** headings, virHashPtr *data) A global '--format=plain|csv|json|xml' flag could apply to every single virsh command, causing these methods to print in the corresponding data format. Thus we'd have no code paths duplicated & flexible data formatting in many styles Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jul 01, 2010 at 03:00:14PM +0100, Daniel P. Berrange wrote:
The changes noted above to make it machine friendly, make it very unfriendly to humans. The patch was focused on making this more friendly for humans.
I would dispute that this makes it "very unfriendly to humans". If you adopted all of my suggestions then the output would be (assuming 1K blocks for sizes): virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available default running yes yes 1921997864 1599875317 319574507 image_dir running yes yes 1921997864 1599875317 319574507 tmp inactive no yes - - - or: virsh # pool-list --details --all -h Name State Autostart Persistent Capacity Allocation Available default running yes yes 1.79T 1.49T 304.77G image_dir running yes yes 1.79T 1.49T 304.77G tmp inactive no yes - - - [Note using "xxGB" is dubious because of the ISO stupid GB vs GiB thing. You have to write either "xxG" or "xxGiB". Coreutils 'df' uses "xxG".]
# virsh list Id Name State ---------------------------------- - f12i686 shut off - f12i686_v1 shut off - f13i686 shut off - f13x86_64 shut off
This is what you have to write in order to get a list of domains from a shell script: guests=$(virsh list --all | tail -n+3 | head -n-1 | awk '{print $2}') and this isn't even reliable for getting other fields such as the state, since that field can contain spaces. [...]
A global '--format=plain|csv|json|xml' flag could apply to every single virsh command, causing these methods to print in the corresponding data format. Thus we'd have no code paths duplicated & flexible data formatting in many styles
But I fully agree if we can do it all in a single way, with no duplicated code paths, and tests for each, then we should do this. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On 07/02/2010 12:00 AM, Daniel P. Berrange wrote: <sip>
A global '--format=plain|csv|json|xml' flag could apply to every single virsh command, causing these methods to print in the corresponding data format. Thus we'd have no code paths duplicated& flexible data formatting in many styles
The concept/idea here seems pretty nifty. It starts putting in place a good foundation for virsh to becoming a widely useful utility. :) We don't even have to implement it all at once. Instead we can convert over one function at a time if that's more practical. We'll probably need another suggested data type as well: virshPrintCapacityValue(some large capacity numeric type) That could take a raw byte (or kilobyte?) value then display it appropriately (ie raw, human readable, etc). ie.: 1979120929996 vs 1,979,120,929,996 vs 1.8TB Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On 07/01/2010 08:38 PM, Richard W.M. Jones wrote:
<snip>
>> virsh # pool-list --details --all
>> Name State Autostart Persistent Capacity Allocation Available
>> ---------------------------------------------------------------------------
>> default running yes yes 1.79 TB 1.49 TB 304.77 GB
>> image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB
>> tmp inactive no yes - - -
<snip>
> * Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work).
Tried it without the embedded spacing today (and submitted patches),
but really not a fan of the output:
***************************************************************************
virsh # pool-list --all --details
Name State Autostart Persistent Capacity Allocation Available
---------------------------------------------------------------------------
default running yes yes 19.69GB 15.97GB 3.71GB
image_dir running yes yes 1.79TB 1.59TB 203.69GB
tmp inactive no yes - - -
***************************************************************************
Would it be workable to retain the space between the columns, instead
changing the "-" value in the "pool-list --details" example like this:
***************************************************************************
virsh # pool-list --all --details
Name State Autostart Persistent Capacity Allocation Available
---------------------------------------------------------------------------
default running yes yes 19.69 GB 15.97 GB 3.71 GB
image_dir running yes yes 1.79 TB 1.59 TB 203.69 GB
tmp inactive no yes -- -- -- -- -- --
***************************************************************************
Thinking this would keep the number of columns consistent, but also be
readable for people.
?
Regards and best wishes,
Justin Clift
--
Salasaga - Open Source eLearning IDE
http://www.salasaga.org
On Sat, Jul 03, 2010 at 07:03:14PM +1000, Justin Clift wrote: > On 07/01/2010 08:38 PM, Richard W.M. Jones wrote: > <snip> > >>virsh # pool-list --details --all > >>Name State Autostart Persistent Capacity Allocation > >>Available > >>--------------------------------------------------------------------------- > >>default running yes yes 1.79 TB 1.49 TB 304.77 > >>GB > >>image_dir running yes yes 1.79 TB 1.49 TB 304.77 > >>GB > >>tmp inactive no yes - - > >>- > <snip> > >* Space within fields "1.79 TB" (awk / sort -k in fact _won't_ work). > > Tried it without the embedded spacing today (and submitted patches), > but really not a fan of the output: > > *************************************************************************** > > virsh # pool-list --all --details > Name State Autostart Persistent Capacity Allocation Available > --------------------------------------------------------------------------- > default running yes yes 19.69GB 15.97GB 3.71GB > image_dir running yes yes 1.79TB 1.59TB 203.69GB > tmp inactive no yes - - - > > *************************************************************************** > > Would it be workable to retain the space between the columns, instead > changing the "-" value in the "pool-list --details" example like this: > > *************************************************************************** > > virsh # pool-list --all --details > Name State Autostart Persistent Capacity Allocation Available > --------------------------------------------------------------------------- > default running yes yes 19.69 GB 15.97 GB 3.71 GB > image_dir running yes yes 1.79 TB 1.59 TB 203.69 GB > tmp inactive no yes -- -- -- -- -- -- > > *************************************************************************** > > Thinking this would keep the number of columns consistent, but also be > readable for people. IMHO your original formatting is the best. Both of these new alternatives looks worse. We should be picking the more human friendly option here and not worry about machine readability for this Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/05/2010 07:01 PM, Daniel P. Berrange wrote: <snip>
Thinking this would keep the number of columns consistent, but also be readable for people.
IMHO your original formatting is the best. Both of these new alternatives looks worse. We should be picking the more human friendly option here and not worry about machine readability for this
Heh, I'm just trying to find a workable middle ground until we put a proper machine readable solution in place (ie cvs, json, etc) ? Regards and best wishes, Justin Clift
Daniel
-- Salasaga - Open Source eLearning IDE http://www.salasaga.org
This patch adds a new --details option to the virsh vol-list command, making its output more useful when many luns are present. Addresses BZ # 605543 https://bugzilla.redhat.com/show_bug.cgi?id=605543 --- This is the v5 patch changed to have a space between the value and unit size for capacity related output. ie: 1.40 TB Additionally, this version copes with storage pools that are not active, unlike the v5 patch. Example output: virsh # vol-list default --details Name Path Type Capacity Allocation -------------------------------------------------------------------------------------------------------------------------------------------------- Fedora-12-x86_64-DVD.iso /var/lib/libvirt/images/Fedora-12-x86_64-DVD.iso file 3.29 GB 3.30 GB Fedora-13-x86_64-DVD.iso /var/lib/libvirt/images/Fedora-13-x86_64-DVD.iso file 3.38 GB 3.38 GB RHEL6.0-20100622.1-Server-i386-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Server-i386-DVD1.iso file 3.42 GB 3.43 GB RHEL6.0-20100622.1-Server-x86_64-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Server-x86_64-DVD1.iso file 3.91 GB 3.91 GB RHEL6.0-20100622.1-Workstation-i386-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Workstation-i386-DVD1.iso file 3.42 GB 3.42 GB RHEL6.0-20100622.1-Workstation-x86_64-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Workstation-x86_64-DVD1.iso file 3.90 GB 3.91 GB virsh # Example output when no volumes are in a pool: virsh # vol-list tmp Name Path ----------------------------------------- virsh # vol-list tmp --details Name Path Type Capacity Allocation -------------------------------------- virsh # tools/virsh.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 4 +- 2 files changed, 232 insertions(+), 31 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index e07fef3..3df5433 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6260,70 +6260,269 @@ static const vshCmdInfo info_vol_list[] = { static const vshCmdOptDef opts_vol_list[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for volumes")}, {NULL, 0, 0, NULL} }; static int cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + virStorageVolInfo volumeInfo; virStoragePoolPtr pool; - int maxactive = 0, i; char **activeNames = NULL; + char *outputStr = NULL; + const char *unit; + double val; + int details = vshCommandOptBool(cmd, "details"); + int numVolumes = 0, i; + int ret, functionReturn; + int stringLength = 0; + size_t allocStrLength = 0, capStrLength = 0; + size_t nameStrLength = 0, pathStrLength = 0; + size_t typeStrLength = 0; + struct volInfoText { + char *allocation; + char *capacity; + char *path; + char *type; + }; + struct volInfoText *volInfoTexts = NULL; + /* Check the connection to libvirtd daemon is still working */ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + /* Look up the pool information given to us by the user */ if (!(pool = vshCommandOptPool(ctl, cmd, "pool", NULL))) return FALSE; - maxactive = virStoragePoolNumOfVolumes(pool); - if (maxactive < 0) { - virStoragePoolFree(pool); - vshError(ctl, "%s", _("Failed to list active vols")); - return FALSE; - } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); + /* Determine the number of volumes in the pool */ + numVolumes = virStoragePoolNumOfVolumes(pool); - if ((maxactive = virStoragePoolListVolumes(pool, activeNames, - maxactive)) < 0) { + /* Retrieve the list of volume names in the pool */ + if (numVolumes > 0) { + activeNames = vshCalloc(ctl, numVolumes, sizeof(*activeNames)); + if ((numVolumes = virStoragePoolListVolumes(pool, activeNames, + numVolumes)) < 0) { vshError(ctl, "%s", _("Failed to list active vols")); VIR_FREE(activeNames); virStoragePoolFree(pool); return FALSE; } - qsort(&activeNames[0], maxactive, sizeof(char *), namesorter); + /* Sort the volume names */ + qsort(&activeNames[0], numVolumes, sizeof(*activeNames), namesorter); + + /* Set aside memory for volume information pointers */ + volInfoTexts = vshCalloc(ctl, numVolumes, sizeof(*volInfoTexts)); } - vshPrintExtra(ctl, "%-20s %-40s\n", _("Name"), _("Path")); - vshPrintExtra(ctl, "-----------------------------------------\n"); - for (i = 0; i < maxactive; i++) { - virStorageVolPtr vol = virStorageVolLookupByName(pool, activeNames[i]); - char *path; + /* Collect the rest of the volume information for display */ + for (i = 0; i < numVolumes; i++) { + /* Retrieve volume info */ + virStorageVolPtr vol = virStorageVolLookupByName(pool, + activeNames[i]); - /* this kind of work with vols is not atomic operation */ - if (!vol) { - VIR_FREE(activeNames[i]); - continue; + /* Retrieve the volume path */ + if ((volInfoTexts[i].path = virStorageVolGetPath(vol)) == NULL) { + /* Something went wrong retrieving a volume path, cope with it */ + volInfoTexts[i].path = vshStrdup(ctl, _("unknown")); } - if ((path = virStorageVolGetPath(vol)) == NULL) { - virStorageVolFree(vol); - continue; - } + /* If requested, retrieve volume type and sizing information */ + if (details) { + if (virStorageVolGetInfo(vol, &volumeInfo) != 0) { + /* Something went wrong retrieving volume info, cope with it */ + volInfoTexts[i].allocation = vshStrdup(ctl, _("unknown")); + volInfoTexts[i].capacity = vshStrdup(ctl, _("unknown")); + volInfoTexts[i].type = vshStrdup(ctl, _("unknown")); + } else { + /* Convert the returned volume info into output strings */ + + /* Volume type */ + if (volumeInfo.type == VIR_STORAGE_VOL_FILE) + volInfoTexts[i].type = vshStrdup(ctl, _("file")); + else + volInfoTexts[i].type = vshStrdup(ctl, _("block")); + + /* Create the capacity output string */ + val = prettyCapacity(volumeInfo.capacity, &unit); + ret = virAsprintf(&volInfoTexts[i].capacity, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Create the allocation output string */ + val = prettyCapacity(volumeInfo.allocation, &unit); + ret = virAsprintf(&volInfoTexts[i].allocation, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + } + + /* Remember the largest length for each output string. + * This lets us displaying header and volume information rows + * using a single, properly sized, printf style output string. + */ + /* Keep the length of name string if longest so far */ + stringLength = strlen(activeNames[i]); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of path string if longest so far */ + stringLength = strlen(volInfoTexts[i].path); + if (stringLength > pathStrLength) + pathStrLength = stringLength; + + /* Keep the length of type string if longest so far */ + stringLength = strlen(volInfoTexts[i].type); + if (stringLength > typeStrLength) + typeStrLength = stringLength; + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(volInfoTexts[i].capacity); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(volInfoTexts[i].allocation); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + } - vshPrint(ctl, "%-20s %-40s\n", - virStorageVolGetName(vol), - path); - VIR_FREE(path); + /* Cleanup memory allocation */ virStorageVolFree(vol); + } + + /* If the --details option wasn't selected, we output the volume + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* The old output format */ + vshPrintExtra(ctl, "%-20s %-40s\n", _("Name"), _("Path")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + for (i = 0; i < numVolumes; i++) { + vshPrint(ctl, "%-20s %-40s\n", activeNames[i], + volInfoTexts[i].path); + } + + /* Cleanup and return */ + functionReturn = TRUE; + goto cleanup; + } + + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Use the length of path header string if it's longest */ + stringLength = strlen(_("Path")); + if (stringLength > pathStrLength) + pathStrLength = stringLength; + + /* Use the length of type header string if it's longest */ + stringLength = strlen(_("Type")); + if (stringLength > typeStrLength) + typeStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Display the string lengths for debugging */ + vshDebug(ctl, 5, "Longest name string = %lu chars\n", nameStrLength); + vshDebug(ctl, 5, "Longest path string = %lu chars\n", pathStrLength); + vshDebug(ctl, 5, "Longest type string = %lu chars\n", typeStrLength); + vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", capStrLength); + vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", allocStrLength); + + /* Create the output template */ + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) pathStrLength, + (unsigned long) typeStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("Path"), _("Type"), + ("Capacity"), _("Allocation")); + for (i = nameStrLength + pathStrLength + typeStrLength + + capStrLength + allocStrLength + + 8; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the volume info rows */ + for (i = 0; i < numVolumes; i++) { + vshPrint(ctl, outputStr, + activeNames[i], + volInfoTexts[i].path, + volInfoTexts[i].type, + volInfoTexts[i].capacity, + volInfoTexts[i].allocation); + } + + /* Cleanup and return */ + functionReturn = TRUE; + goto cleanup; + +asprintf_failure: + + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); + } + functionReturn = FALSE; + +cleanup: + + /* Safely free the memory allocated in this function */ + for (i = 0; i < numVolumes; i++) { + /* Cleanup the memory for one volume info structure per loop */ + VIR_FREE(volInfoTexts[i].path); + VIR_FREE(volInfoTexts[i].type); + VIR_FREE(volInfoTexts[i].capacity); + VIR_FREE(volInfoTexts[i].allocation); VIR_FREE(activeNames[i]); } + + /* Cleanup remaining memory */ + VIR_FREE(outputStr); + VIR_FREE(volInfoTexts); VIR_FREE(activeNames); virStoragePoolFree(pool); - return TRUE; + + /* Return the desired value */ + return functionReturn; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 2b2227f..b2dff8b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -850,10 +850,12 @@ Returns basic information about the given storage volume. I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool the volume is in. I<vol-name-or-key-or-path> is the name or key or path of the volume to return information for. -=item B<vol-list> I<--pool> I<pool-or-uuid> +=item B<vol-list> [optional I<--pool>] I<pool-or-uuid> optional I<--details> Return the list of volumes in the given storage pool. I<--pool> I<pool-or-uuid> is the name or UUID of the storage pool. +The I<--details> option instructs virsh to additionally display volume +type and capacity related information where available. =item B<vol-pool> [optional I<--uuid>] I<vol-key-or-path> -- 1.7.1
On Tue, Jul 06, 2010 at 12:16:13AM +1000, Justin Clift wrote:
This patch adds a new --details option to the virsh vol-list command, making its output more useful when many luns are present.
Addresses BZ # 605543
https://bugzilla.redhat.com/show_bug.cgi?id=605543
---
This is the v5 patch changed to have a space between the value and unit size for capacity related output. ie:
1.40 TB
Additionally, this version copes with storage pools that are not active, unlike the v5 patch.
Example output:
virsh # vol-list default --details Name Path Type Capacity Allocation -------------------------------------------------------------------------------------------------------------------------------------------------- Fedora-12-x86_64-DVD.iso /var/lib/libvirt/images/Fedora-12-x86_64-DVD.iso file 3.29 GB 3.30 GB Fedora-13-x86_64-DVD.iso /var/lib/libvirt/images/Fedora-13-x86_64-DVD.iso file 3.38 GB 3.38 GB RHEL6.0-20100622.1-Server-i386-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Server-i386-DVD1.iso file 3.42 GB 3.43 GB RHEL6.0-20100622.1-Server-x86_64-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Server-x86_64-DVD1.iso file 3.91 GB 3.91 GB RHEL6.0-20100622.1-Workstation-i386-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Workstation-i386-DVD1.iso file 3.42 GB 3.42 GB RHEL6.0-20100622.1-Workstation-x86_64-DVD1.iso /var/lib/libvirt/images/RHEL6.0-20100622.1-Workstation-x86_64-DVD1.iso file 3.90 GB 3.91 GB
virsh #
Example output when no volumes are in a pool:
virsh # vol-list tmp Name Path -----------------------------------------
virsh # vol-list tmp --details Name Path Type Capacity Allocation --------------------------------------
virsh #
tools/virsh.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 4 +- 2 files changed, 232 insertions(+), 31 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/07/2010 09:34 PM, Daniel P. Berrange wrote: <snip>
tools/virsh.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 4 +- 2 files changed, 232 insertions(+), 31 deletions(-)
ACK
Daniel
Pushed. Regards and best wishes, Justin Clift
On Tue, Jun 22, 2010 at 02:24:31PM -0400, Hugh O. Brock wrote:
On Tue, Jun 22, 2010 at 06:54:55PM +0100, Daniel P. Berrange wrote:
On Tue, Jun 22, 2010 at 11:44:57AM -0600, Eric Blake wrote:
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?
Correct, we shouldn't change this behaviour - it'll break apps parsing the output
FWIW Rich Jones complains that the output as it stands is nigh on unparseable anyway. Perhaps we should consider that a bug, and fix it...
That's unrelated to this patch really. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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
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 --- Updated version of the patch. This one has the original "return FALSE" style behaviour if no pools were found, plus the sizeof() and array type/allocations fixed that Eric pointed out. Also renamed the "function_ret" variable to functionReturn for consistency, plus other minor tweaks. Example output: virsh # pool-list Name State Autostart ----------------------------------------- default active yes image_dir active yes virsh # pool-list --all Name State Autostart ----------------------------------------- default active yes image_dir active yes tmp inactive no virsh # pool-list --details Name State Autostart Persistent Capacity Allocation Available -------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB virsh # pool-list --details --all Name State Autostart Persistent Capacity Allocation Available --------------------------------------------------------------------------- default running yes yes 1.79 TB 1.49 TB 304.77 GB image_dir running yes yes 1.79 TB 1.49 TB 304.77 GB tmp inactive no yes - - - virsh # tools/virsh.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 6 +- 2 files changed, 360 insertions(+), 67 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b0c6a76..7973c0b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4891,114 +4891,405 @@ static const vshCmdInfo info_pool_list[] = { static const vshCmdOptDef opts_pool_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive pools")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active pools")}, + {"details", VSH_OT_BOOL, 0, N_("display extended details for pools")}, {NULL, 0, 0, NULL} }; static int cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { - int inactive = vshCommandOptBool(cmd, "inactive"); + virStoragePoolInfo info; + char **poolNames = NULL; + int i, functionReturn, ret; + int numActivePools = 0, numInactivePools = 0, numAllPools = 0; + size_t stringLength = 0, nameStrLength = 0; + size_t autostartStrLength = 0, persistStrLength = 0; + size_t stateStrLength = 0, capStrLength = 0; + size_t allocStrLength = 0, availStrLength = 0; + struct poolInfoText { + char *state; + char *autostart; + char *persistent; + char *capacity; + char *allocation; + char *available; + }; + struct poolInfoText *poolInfoTexts = NULL; + + /* Determine the options passed by the user */ int all = vshCommandOptBool(cmd, "all"); + int details = vshCommandOptBool(cmd, "details"); + int inactive = vshCommandOptBool(cmd, "inactive"); int active = !inactive || all ? 1 : 0; - int maxactive = 0, maxinactive = 0, i; - char **activeNames = NULL, **inactiveNames = NULL; inactive |= all; + /* Check the connection to libvirtd daemon is still working */ if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + /* Retrieve the number of active storage pools */ if (active) { - maxactive = virConnectNumOfStoragePools(ctl->conn); - if (maxactive < 0) { + numActivePools = virConnectNumOfStoragePools(ctl->conn); + if (numActivePools < 0) { vshError(ctl, "%s", _("Failed to list active pools")); return FALSE; } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); - - if ((maxactive = virConnectListStoragePools(ctl->conn, activeNames, - maxactive)) < 0) { - vshError(ctl, "%s", _("Failed to list active pools")); - VIR_FREE(activeNames); - return FALSE; - } - - qsort(&activeNames[0], maxactive, sizeof(char *), namesorter); - } } + + /* Retrieve the number of inactive storage pools */ if (inactive) { - maxinactive = virConnectNumOfDefinedStoragePools(ctl->conn); - if (maxinactive < 0) { + numInactivePools = virConnectNumOfDefinedStoragePools(ctl->conn); + if (numInactivePools < 0) { vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(activeNames); return FALSE; } - if (maxinactive) { - inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive); + } - if ((maxinactive = virConnectListDefinedStoragePools(ctl->conn, inactiveNames, maxinactive)) < 0) { - vshError(ctl, "%s", _("Failed to list inactive pools")); - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return FALSE; - } + /* Determine the total number of pools to list */ + numAllPools = numActivePools + numInactivePools; + if (!numAllPools) { + /* No pools to list, so cleanup and return */ + vshPrint(ctl, "%s", _("Failed to list any pools")); + return FALSE; + } - qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter); + /* Allocate memory for arrays of storage pool names and info */ + poolNames = vshCalloc(ctl, numAllPools, sizeof(*poolNames)); + poolInfoTexts = + vshCalloc(ctl, numAllPools, sizeof(*poolInfoTexts)); + + /* Retrieve a list of active storage pool names */ + if (active) { + if ((virConnectListStoragePools(ctl->conn, + poolNames, numActivePools)) < 0) { + vshError(ctl, "%s", _("Failed to list active pools")); + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + return FALSE; } } - vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); - for (i = 0; i < maxactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, activeNames[i]); - const char *autostartStr; - int autostart = 0; + /* Add the inactive storage pools to the end of the name list */ + if (inactive) { + if ((virConnectListDefinedStoragePools(ctl->conn, + &poolNames[numActivePools], + numInactivePools)) < 0) { + vshError(ctl, "%s", _("Failed to list inactive pools")); + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + return FALSE; + } + } + + /* Sort the storage pool names */ + qsort(poolNames, numAllPools, sizeof(*poolNames), namesorter); - /* this kind of work with pools is not atomic operation */ + /* Collect the storage pool information for display */ + for (i = 0; i < numAllPools; i++) { + int autostart = 0, persistent = 0; + + /* Retrieve a pool object, looking it up by name */ + virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, + poolNames[i]); if (!pool) { - VIR_FREE(activeNames[i]); + VIR_FREE(poolNames[i]); continue; } + /* Retrieve the autostart status of the pool */ if (virStoragePoolGetAutostart(pool, &autostart) < 0) - autostartStr = _("no autostart"); + poolInfoTexts[i].autostart = vshStrdup(ctl, _("no autostart")); else - autostartStr = autostart ? _("yes") : _("no"); + poolInfoTexts[i].autostart = vshStrdup(ctl, autostart ? + _("yes") : _("no")); + + /* Retrieve the persistence status of the pool */ + if (details) { + persistent = virStoragePoolIsPersistent(pool); + vshDebug(ctl, 5, "Persistent flag value: %d\n", persistent); + if (persistent < 0) + poolInfoTexts[i].persistent = vshStrdup(ctl, _("unknown")); + else + poolInfoTexts[i].persistent = vshStrdup(ctl, persistent ? + _("yes") : _("no")); - vshPrint(ctl, "%-20s %-10s %-10s\n", - virStoragePoolGetName(pool), - _("active"), - autostartStr); + /* Keep the length of persistent string if longest so far */ + stringLength = strlen(poolInfoTexts[i].persistent); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + } + + /* 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")); + if (details) { + poolInfoTexts[i].capacity = vshStrdup(ctl, _("unknown")); + poolInfoTexts[i].allocation = vshStrdup(ctl, _("unknown")); + poolInfoTexts[i].available = vshStrdup(ctl, _("unknown")); + } + } else { + /* Decide which state string to display */ + if (details) { + /* --details option was specified, we're using detailed state + * strings */ + switch (info.state) { + case VIR_STORAGE_POOL_INACTIVE: + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + break; + case VIR_STORAGE_POOL_BUILDING: + poolInfoTexts[i].state = vshStrdup(ctl, _("building")); + break; + case VIR_STORAGE_POOL_RUNNING: + poolInfoTexts[i].state = vshStrdup(ctl, _("running")); + break; + case VIR_STORAGE_POOL_DEGRADED: + poolInfoTexts[i].state = vshStrdup(ctl, _("degraded")); + break; + case VIR_STORAGE_POOL_INACCESSIBLE: + poolInfoTexts[i].state = vshStrdup(ctl, _("inaccessible")); + break; + } + + /* Create the pool size related strings */ + if (info.state == VIR_STORAGE_POOL_RUNNING || + info.state == VIR_STORAGE_POOL_DEGRADED) { + double val; + const char *unit; + + /* Create the capacity output string */ + val = prettyCapacity(info.capacity, &unit); + ret = virAsprintf(&poolInfoTexts[i].capacity, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Create the allocation output string */ + val = prettyCapacity(info.allocation, &unit); + ret = virAsprintf(&poolInfoTexts[i].allocation, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Create the available space output string */ + val = prettyCapacity(info.available, &unit); + ret = virAsprintf(&poolInfoTexts[i].available, + "%.2lf %s", val, unit); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + } else { + /* Capacity related information isn't available */ + poolInfoTexts[i].capacity = vshStrdup(ctl, _("-")); + poolInfoTexts[i].allocation = vshStrdup(ctl, _("-")); + poolInfoTexts[i].available = vshStrdup(ctl, _("-")); + } + + /* Keep the length of capacity string if longest so far */ + stringLength = strlen(poolInfoTexts[i].capacity); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Keep the length of allocation string if longest so far */ + stringLength = strlen(poolInfoTexts[i].allocation); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Keep the length of available string if longest so far */ + stringLength = strlen(poolInfoTexts[i].available); + if (stringLength > availStrLength) + availStrLength = stringLength; + } else { + /* --details option was not specified, only active/inactive + * state strings are used */ + if (info.state == VIR_STORAGE_POOL_INACTIVE) + poolInfoTexts[i].state = vshStrdup(ctl, _("inactive")); + else + poolInfoTexts[i].state = vshStrdup(ctl, _("active")); + } + } + + /* Keep the length of name string if longest so far */ + stringLength = strlen(poolNames[i]); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Keep the length of state string if longest so far */ + stringLength = strlen(poolInfoTexts[i].state); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Keep the length of autostart string if longest so far */ + stringLength = strlen(poolInfoTexts[i].autostart); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Free the pool object */ virStoragePoolFree(pool); - VIR_FREE(activeNames[i]); } - for (i = 0; i < maxinactive; i++) { - virStoragePoolPtr pool = virStoragePoolLookupByName(ctl->conn, inactiveNames[i]); - const char *autostartStr; - int autostart = 0; - /* this kind of work with pools is not atomic operation */ - if (!pool) { - VIR_FREE(inactiveNames[i]); - continue; + /* If the --details option wasn't selected, we output the pool + * info using the fixed string format from previous versions to + * maintain backward compatibility. + */ + + /* Output basic info then return if --details option not selected */ + if (!details) { + /* Output old style header */ + vshPrintExtra(ctl, "%-20s %-10s %-10s\n", _("Name"), _("State"), + _("Autostart")); + vshPrintExtra(ctl, "-----------------------------------------\n"); + + /* Output old style pool info */ + for (i = 0; i < numAllPools; i++) { + vshPrint(ctl, "%-20s %-10s %-10s\n", + poolNames[i], + poolInfoTexts[i].state, + poolInfoTexts[i].autostart); } - if (virStoragePoolGetAutostart(pool, &autostart) < 0) - autostartStr = _("no autostart"); - else - autostartStr = autostart ? _("yes") : _("no"); + /* Cleanup and return */ + functionReturn = TRUE; + goto cleanup; + } - vshPrint(ctl, "%-20s %-10s %-10s\n", - inactiveNames[i], - _("inactive"), - autostartStr); + /* We only get here if the --details option was selected. */ + + /* Use the length of name header string if it's longest */ + stringLength = strlen(_("Name")); + if (stringLength > nameStrLength) + nameStrLength = stringLength; + + /* Use the length of state header string if it's longest */ + stringLength = strlen(_("State")); + if (stringLength > stateStrLength) + stateStrLength = stringLength; + + /* Use the length of autostart header string if it's longest */ + stringLength = strlen(_("Autostart")); + if (stringLength > autostartStrLength) + autostartStrLength = stringLength; + + /* Use the length of persistent header string if it's longest */ + stringLength = strlen(_("Persistent")); + if (stringLength > persistStrLength) + persistStrLength = stringLength; + + /* Use the length of capacity header string if it's longest */ + stringLength = strlen(_("Capacity")); + if (stringLength > capStrLength) + capStrLength = stringLength; + + /* Use the length of allocation header string if it's longest */ + stringLength = strlen(_("Allocation")); + if (stringLength > allocStrLength) + allocStrLength = stringLength; + + /* Use the length of available header string if it's longest */ + stringLength = strlen(_("Available")); + if (stringLength > availStrLength) + availStrLength = stringLength; + + /* Display the string lengths for debugging. */ + vshDebug(ctl, 5, "Longest name string = %lu chars\n", + (unsigned long) nameStrLength); + vshDebug(ctl, 5, "Longest state string = %lu chars\n", + (unsigned long) stateStrLength); + vshDebug(ctl, 5, "Longest autostart string = %lu chars\n", + (unsigned long) autostartStrLength); + vshDebug(ctl, 5, "Longest persistent string = %lu chars\n", + (unsigned long) persistStrLength); + vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", + (unsigned long) capStrLength); + vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", + (unsigned long) allocStrLength); + vshDebug(ctl, 5, "Longest available string = %lu chars\n", + (unsigned long) availStrLength); + + /* Create the output template. Each column is sized according to + * the longest string. + */ + char *outputStr; + ret = virAsprintf(&outputStr, + "%%-%lus %%-%lus %%-%lus %%-%lus %%%lus %%%lus %%%lus\n", + (unsigned long) nameStrLength, + (unsigned long) stateStrLength, + (unsigned long) autostartStrLength, + (unsigned long) persistStrLength, + (unsigned long) capStrLength, + (unsigned long) allocStrLength, + (unsigned long) availStrLength); + if (ret < 0) { + /* An error occurred creating the string, return */ + goto asprintf_failure; + } + + /* Display the header */ + vshPrint(ctl, outputStr, _("Name"), _("State"), _("Autostart"), + _("Persistent"), _("Capacity"), _("Allocation"), _("Available")); + for (i = nameStrLength + stateStrLength + autostartStrLength + + persistStrLength + capStrLength + + allocStrLength + availStrLength + + 12; i > 0; i--) + vshPrintExtra(ctl, "-"); + vshPrintExtra(ctl, "\n"); + + /* Display the pool info rows */ + for (i = 0; i < numAllPools; i++) { + vshPrint(ctl, outputStr, + poolNames[i], + poolInfoTexts[i].state, + poolInfoTexts[i].autostart, + poolInfoTexts[i].persistent, + poolInfoTexts[i].capacity, + poolInfoTexts[i].allocation, + poolInfoTexts[i].available); + } + + /* Cleanup and return */ + functionReturn = TRUE; + goto cleanup; - virStoragePoolFree(pool); - VIR_FREE(inactiveNames[i]); +asprintf_failure: + + /* Display an appropriate error message then cleanup and return */ + switch (errno) { + case ENOMEM: + /* Couldn't allocate memory */ + vshError(ctl, "%s", _("Out of memory")); + break; + default: + /* Some other error */ + vshError(ctl, _("virAsprintf failed (errno %d)"), errno); } - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return TRUE; + functionReturn = FALSE; + +cleanup: + + /* Safely free the memory allocated in this function */ + for (i = 0; i < numAllPools; i++) { + /* Cleanup the memory for one pool info structure */ + VIR_FREE(poolInfoTexts[i].state); + VIR_FREE(poolInfoTexts[i].autostart); + VIR_FREE(poolInfoTexts[i].persistent); + VIR_FREE(poolInfoTexts[i].capacity); + VIR_FREE(poolInfoTexts[i].allocation); + VIR_FREE(poolInfoTexts[i].available); + VIR_FREE(poolNames[i]); + } + + /* Cleanup the memory for the initial arrays*/ + VIR_FREE(poolInfoTexts); + VIR_FREE(poolNames); + + /* Return the desired value */ + return functionReturn; } /* diff --git a/tools/virsh.pod b/tools/virsh.pod index 6e01a89..2b2227f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -743,11 +743,13 @@ variables, and defaults to C<vi>. Returns basic information about the I<pool> object. -=item B<pool-list> optional I<--inactive> I<--all> +=item B<pool-list> optional I<--inactive> I<--all> I<--details> List pool objects known to libvirt. By default, only pools in use by active domains are listed; I<--inactive> lists just the inactive -pools, and I<--all> lists all pools. +pools, and I<--all> lists all pools. The I<--details> option instructs +virsh to additionally display pool persistence and capacity related +information where available. =item B<pool-name> I<uuid> -- 1.7.0.1
On 06/28/2010 10:47 AM, 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
---
plus the sizeof() and array type/allocations fixed that Eric pointed out. Also renamed the "function_ret" variable to functionReturn for consistency, plus other minor tweaks.
ACK to these cleanups.
Updated version of the patch. This one has the original "return FALSE" style behaviour if no pools were found,
Nope, that's not the original behavior. $ virsh -c test:///default pool-list --inactive; echo $? Name State Autostart ----------------------------------------- 0 $ tools/virsh -c test:///default pool-list --inactive; echo $? Failed to list any pools 1 We really DO want to give 0 exit status, after listing a header but no clients. However, the rest of your patch is in good shape, so I squashed this in: diff --git i/tools/virsh.c w/tools/virsh.c index 7973c0b..de4876f 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -4947,11 +4947,6 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* Determine the total number of pools to list */ numAllPools = numActivePools + numInactivePools; - if (!numAllPools) { - /* No pools to list, so cleanup and return */ - vshPrint(ctl, "%s", _("Failed to list any pools")); - return FALSE; - } /* Allocate memory for arrays of storage pool names and info */ poolNames = vshCalloc(ctl, numAllPools, sizeof(*poolNames)); then pushed the result. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/30/2010 12:38 AM, Eric Blake wrote: <snip>
We really DO want to give 0 exit status, after listing a header but no clients.
However, the rest of your patch is in good shape, so I squashed this in:
Thanks Eric. Got it. :) The output does look better now if there weren't any pools too. Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
participants (7)
-
Daniel P. Berrange -
Dave Allan -
Eric Blake -
Hugh O. Brock -
Justin Clift -
Justin Clift -
Richard W.M. Jones