Re: [Qemu-devel] [PATCH v4 02/11] block: Accept node-name for block-commit

[Kevin Wolf <kwolf@redhat.com>]

Am 01.08.2016 um 15:35 hat Alberto Garcia geschrieben:
> On Thu 14 Jul 2016 03:28:05 PM CEST, Kevin Wolf wrote:
> > -    blk = blk_by_name(device);
> > -    if (!blk) {
> > -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > -                  "Device '%s' not found", device);
> > +    bs = qmp_get_root_bs(device, &local_err);
> > +    if (!bs) {
> > +        bs = bdrv_lookup_bs(device, device, NULL);
> > +        if (!bs) {
> > +            error_free(local_err);
> > +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > +                      "Device '%s' not found", device);
> > +        } else {
> > +            error_propagate(errp, local_err);
> > +        }
> >          return;
> 
> It seems that you're calling bdrv_lookup_bs() here twice, once in
> qmp_get_root_bs() and then again directly. If the purpose is to see
> whether the error is "device not found" or "device is not a root node" I
> think the code would be clearer if you do everything here.

Hm, I would like every command that needs a root node to go through
qmp_get_root_bs() for consistency. You're right that the extra code is
just for checking which error class we need to use.

> On a related note, you're keeping the DeviceNotFound error here, but not
> in block-stream. Wouldn't it be better to keep both APIs consistent?

This is the only instance that libvirt actually makes use of, so we have
to keep it. The others just happened to use the error class for no
specific reason, so they should have been GenericError from the start.

Kevin