Re: [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