qemu-block
[Top][All Lists]
Advanced

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

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


From: Kevin Wolf
Subject: Re: [Qemu-block] [PATCH v4 02/11] block: Accept node-name for block-commit
Date: Tue, 2 Aug 2016 18:22:49 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

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



reply via email to

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