3:30 a.m.
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
11:23 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:29 a.m.
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
11:59 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:08 p.m.
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
8:45 a.m.
New subject: [libvirt] [PATCHv3 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
---
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
8:52 a.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: add new --details option to pool-list
On 06/21/2010 11:45 PM, Justin Clift wrote:
<snip>
Submitting this patch for review by itself, rather than with the
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
4:47 p.m.
New subject: [libvirt] [PATCHv4 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
---
(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
12:44 p.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
On 06/21/2010 03:47 PM, Justin Clift wrote:
This patch adds a new --details option to the virsh pool-list
command, making its output more useful to people who use virsh
for significant lengths of time.
Addresses BZ # 605543
https://bugzilla.redhat.com/show_bug.cgi?id=605543
+ /* Determine the total number of pools to list */
+ numAllPools = numActivePools + numInactivePools;
+ if (!numAllPools) {
+ /* No pools to list, so cleanup and return */
+ vshPrint(ctl, "%s\n", _("No matching pools to display"));
+ return TRUE;
+ }
This is a change in behavior, even when the user didn't specify
--details. Might it cause any backward compatibility issues in
scripting, or are we okay with the change?
$ old-virsh -c test:///default pool-list --inactive
Name State Autostart
-----------------------------------------
$ tools/virsh -c test:///default pool-list --inactive
No matching pools to display
$
- qsort(&inactiveNames[0], maxinactive, sizeof(char*), namesorter);
+ /* Allocate memory for arrays of storage pool names and info */
+ poolNames = vshCalloc(ctl, numAllPools, sizeof(char *));
Rather than sizeof(char *), I'd rather see sizeof(*poolNames), which
isolates us from problems if we ever change the type of poolNames.
+ poolInfoTexts =
+ vshCalloc(ctl, numAllPools, sizeof(struct poolInfoText *));
Likewise, sizeof(*poolInfoTexts)
+
+ /* Sort the storage pool names */
+ qsort(poolNames, numAllPools, sizeof(char *), namesorter);
Again, sizeof(*poolNames)
Hmm, this changes existing output of --all, in that it now intermixes
lines for active and inactive pools, but I'm okay with that change (that
is, you swapped the primary sort key from being status over to being name).
+ /* Allocate and clear memory for one row of pool info */
+ poolInfoTexts[i] = vshCalloc(ctl, 1, sizeof(struct poolInfoText));
Why did we allocate an array of poolInfoText*, only to then allocate
each poolInfoText? I'd rather see poolInfoTexts be (struct
poolInfoText*) than (struct poolInfoText**), so that allocating the
array up front is good enough; we don't have to do further allocation
each time through the loop.
+ /* Collect further extended information about the pool */
+ if (virStoragePoolGetInfo(pool, &info) != 0) {
+ /* Something went wrong retrieving pool info, cope with it */
+ vshError(ctl, "%s", _("Could not retrieve pool
information"));
+ poolInfoTexts[i]->state = vshStrdup(ctl, _("unknown"));
+ poolInfoTexts[i]->capacity = vshStrdup(ctl, _("unknown"));
+ poolInfoTexts[i]->allocation = vshStrdup(ctl, _("unknown"));
+ poolInfoTexts[i]->available = vshStrdup(ctl, _("unknown"));
We could surround the last three vshStrdup with if (details), to avoid
malloc pressure for the "unknown" answer that would not be printed.
+ /* Display the header */
+ vshPrint(ctl, outputStr, _("Name"), _("State"),
_("Autostart"),
+ _("Persistent"), ("Capacity"),
_("Allocation"), _("Available"));
Missing _() around Capacity.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:54 p.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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 :|
1:24 p.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
1:35 p.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:58 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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 :|
5:24 a.m.
New subject: [libvirt] Improving virsh for scripting: easily parseable output?
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
5:42 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
9:51 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
10:06 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
10:41 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
9:59 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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:
http://libguestfs.org/virt-df.1.html#note_about_csv_format
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
10:11 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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:
>
> http://libguestfs.org/virt-df.1.html#note_about_csv_format
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
10:17 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
10:31 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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/
5:38 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
5:50 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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 :|
7:57 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:01 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-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 :|
8:08 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-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
Oh, excellent!
:)
--Hugh
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:44 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-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
9 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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 :|
9:46 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
9:54 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
4:03 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
4:01 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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 :|
6:33 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
9:16 a.m.
New subject: [libvirt] [PATCHv6 2/2] virsh: add new --details option to vol-list
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
6:34 a.m.
New subject: [libvirt] [PATCHv6 2/2] virsh: add new --details option to vol-list
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 :|
9:47 a.m.
New subject: [libvirt] [PATCHv6 2/2] virsh: add new --details option to vol-list
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
3:59 a.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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 :|
5:13 p.m.
New subject: [libvirt] [PATCHv4 1/2] virsh: add new --details option to pool-list
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
11:47 a.m.
New subject: [libvirt] [PATCHv5 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
---
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
9:38 a.m.
New subject: [libvirt] [PATCHv5 1/2] virsh: add new --details option to pool-list
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:41 a.m.
New subject: [libvirt] [PATCHv5 1/2] virsh: add new --details option to pool-list
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
5253
days inactive
5274
days old
41 comments
7 participants
participants (7)
-
Daniel P. Berrange -
Dave Allan -
Eric Blake -
Hugh O. Brock -
Justin Clift -
Justin Clift -
Richard W.M. Jones