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@xxxxxxxxxx> > > > > --- > > [...] > > > > > + > > > > + 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
Attachment:
signature.asc
Description: PGP signature
