Re: [libvirt PATCH 4/6] conf: implement support for vhostuser disk
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(a)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.
/* 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.