qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [RFC 0/3] block: Error parameter for opening/creating i


From: Kevin Wolf
Subject: Re: [Qemu-devel] [RFC 0/3] block: Error parameter for opening/creating images
Date: Thu, 5 Sep 2013 12:40:20 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 05.09.2013 um 10:10 hat Max Reitz geschrieben:
> This RFC adds an Error ** parameter to bdrv_open, bdrv_file_open,
> bdrv_create and the respective functions provided by a block driver.
> 
> This results in more specific error information than just -errno provided
> to the user when opening or creating images (disregarding the fact that
> block drivers often already use error_report, which is generally changed
> to error_setg through this patch).
> 
> The last patch in this series changes the qcow2 block driver to set an
> example of usage in a block driver.
> 
> Note that several I/O tests break by applying this RFC since they expect
> different error messages (generally, previously, an error message on
> image opening/creation consisted of two lines; the first of which would be
> generated by the driver through error_report, the second by the block
> layer itself through strerror(-ret); this patch is designed to merge these
> two lines into a single one). This applies to the tests 49, 51, 54 and 60.
> 
> Max Reitz (3):
>   bdrv: Use "Error" for opening images
>   block: Error parameter for opening functions
>   qcow2: Use Error parameter

One thing I'd like to suggest is to keep the conversions of
bdrv_(file_)open and bdrv_create separate. I don't think there are
dependencies between them, so two logical changes should come in two
separate patches.

The other point is that you do a lot of "dummy" conversions where you
pass NULL as errp. This is generally not what we want to have in the end
because it swallows error messages that would previously be printed by
error_report(). I guess most of them are changed into real error
handling, but it's hard to keep track of the places that need to be
fixed in later patches.

In order to address this, I think it would be better to use code like
this when you can't forward the errors yet:

Error *local_err;
foo(&local_err);
if (error_is_set(&local_err)) {
    qerror_report_err(local_err);
    error_free(local_err);
}

So that the output isn't lost in any intermediate commit of your series.

Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]