Re: [libvirt] [PATCH] Better error reporting in virsh.
On 04/05/2010 05:20 PM, Eric Blake wrote:
On 04/05/2010 11:37 AM, Chris Lalancette wrote:
> This patch remedies the situation somewhat by
> printing out the errors inside the command methods
> themselves when we know it will go through a cleanup
> path that will lose the error.
Sounds reasonable for purposes of better reporting, but it also sounds a
bit fragile - is it always easy to tell what might be clearing the error
later? Is this something where we might inadvertently break things by
rewriting a cleanup: label, and end up with duplicate messages down the
road?
Yeah, I agree it is fragile. We need to come up with a better way to do this.
Maybe in the error path we can save any "previous" error before we call the
cleanup functions, and then restore it afterwards? I'm not sure. In any
case I think it's a separate patch.
> @@ -2623,9 +2623,8 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd)
> format = vshCommandOptString(cmd, "format", NULL);
> configFile = vshCommandOptString(cmd, "config", NULL);
>
> - if (virFileReadAll(configFile, 1024*1024, &configData) < 0) {
> + if (virFileReadAll(configFile, 1024*1024, &configData) < 0)
> return FALSE;
> - }
Why the unrelated no-op change here, and in the next couple of hunks?
I was just auditing all of the virFileReadAll uses, and I saw that these
didn't conform to the "usual" style in libvirt. I just fixed them at
the same time.
> @@ -5427,6 +5424,7 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd)
> }
>
> if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
> + virshReportError(ctl);
> virStoragePoolFree(pool);
> return FALSE;
Should we add comments that remind us that we know that
virStoragePoolFree will clear any errors? Likewise for the other
changes in this file?
Yeah, again, this whole thing is pretty fragile. We'll come up with
a better solution.
At any rate, ACK that it improves the virsh experience.
Thanks. I've pushed this patch as-is, and we'll do further improvements
in additional patches.
--
Chris Lalancette