Re: [Qemu-devel] [PATCH V7 05/14] block: add snapshot info query function bdrv_query_snapshot_infolist()

[Stefan Hajnoczi]

On Thu, Feb 28, 2013 at 09:53:53AM +0800, Wenchao Xia wrote:
> 于 2013-2-27 22:26, Markus Armbruster 写道:
> > Wenchao Xia <address@hidden> writes:
> > 
> >>    This patch add function bdrv_query_snapshot_infolist(), which will
> > 
> > adds
> > 
> >> return snapshot info of an image in qmp object format. The implementation
> >> code are based on the code moved from qemu-img.c with modification to fit 
> >> more
> > 
> > implementation is based
> > 
> >> for qmp based block layer API.
> >>    To help filter out snapshot info not needed, a call back function is
> >> added in bdrv_query_snapshot_infolist().
> >>    bdrv_can_read_snapshot() should be called before call this function,
> >> to avoid got *errp set unexpectedly.
> > 
> > "avoid getting"
> > 
> > Not sure about "should".  Couldn't a caller safely call this with a
> > non-snapshot bs argument, and simply deal with the error?
> > 
>   yes, but in some case this error should be avoided. For eg, in
> bdrv_query_image_info(), if a image is not inserted, or it can't
> take snapshot, other info should also be returned without error,
> so if bdrv_can_read_snapshot() is called before, this kind of
> error is avoided, and caller can check if there is other
> error. I guess it should be documented as "in some case,
> bdrv_can_read_snapshot() should be called."

Is the problem that we cannot test what an Error instance means?  So the
caller cannot test the Error to find out whether the medium is not
inserted.

It's cleaner to keep a -errno return value *plus* have an Error*
argument.  That way QEMU callers can distinguish the error meaning and
we don't have to create a separate function that gets called first to
test very specific conditions that aren't meaningful by themselves.

Stefan