[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Better error reporting in virsh.



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?

> @@ -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?

> @@ -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?

At any rate, ACK that it improves the virsh experience.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]