[libvirt PATCH 4/6] conf: implement support for vhostuser disk

[Pavel Hrdina]

On Wed, Feb 03, 2021 at 11:08:41AM +0100, Peter Krempa wrote:
> On Wed, Feb 03, 2021 at 10:51:23 +0100, Pavel Hrdina wrote:
> > On Wed, Feb 03, 2021 at 10:27:01AM +0100, Peter Krempa wrote:
> > > On Tue, Feb 02, 2021 at 16:04:10 +0100, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > > > ---
> 
> [...]
> 
> > > > +
> > > > +    src->vhostuser->type = virDomainChrTypeFromString(type);
> > > > +    src->vhostuser->data.nix.path = g_steal_pointer(&path);
> > > 
> > > 'path' doesn't really need a temp variable.
> > 
> > True, but IMHO it makes the code more readable. Without the variable it
> > would look like this:
> > 
> >     g_autofree char *type = virXMLPropString(node, "type");
> 
> [...]
> 
> > 
> > which is mixing the checks and assignment together.
> 
> Agreed, keep it as is.
> 
> > > > +
> 
> [...]
> 
> 
> > > > +    /* Unsupported driver elements */
> > > 
> > > s/driver/virtio/ ?
> > 
> > This was future proofing the comment :) currently there is only single
> > child element <virtio>. So I would be OK with both versions:
> > 
> >     /* Unsupported driver child elements */
> 
> You'd have to move the image cache setting here, since that's also an
> subelement.

Somehow I missed that the 'metadata_cache' is also an element. I'll move
it and keep this comment.

> > 
> >     /* Unsupported virtio element */
> 
> This is okay
> 
> > > [...]
> > > 
> > > > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> > > > index 6456100170..f6e81a7503 100644
> > > > --- a/src/qemu/qemu_block.c
> > > > +++ b/src/qemu/qemu_block.c
> > > > @@ -1173,6 +1173,9 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
> > > >              return NULL;
> > > >          break;
> > > >  
> > > > +    case VIR_STORAGE_TYPE_VHOST_USER:
> > > > +        break;
> > > 
> > > This case must return NULL and an error per API contract of the function.
> > 
> > Fixed. It should never happen but I agree that we should make sure to
> > error out if it happens.
> 
> Adding the error also prevents potential crash in such case, since
> 'fileprops' would still be NULL, but code after that adding the common
> props expects it to be already allocated.

Right, another reason.

Fixed for v2.

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210203/5aaea203/attachment-0001.sig>