Re: [PATCHv2 1/5] qemu: Add support for NVMe namespace disk bus type
Thanks a lot for the detailed review and suggestions.
Reviewing this would likely be simpler if addition of the controller
was split from the addition of the disk.
Okay, I will split the patch into two parts as suggested—one for introducing the
controller and another for the disk.
We ususally defer validation of minor infractions such as this to the
schema rather than having code for it.
I will remove the validation logic and keep only the logic for retrieving the
'serial' value.
You have a switch statement checking type right below ...
... so remove all of the above for:
def->opts.nvmeopts.serial = virXPathString("string(./serial)", ctxt);
ACK
User provided strings must be formatted using 'virBufferEscapeString'
to ensure proper XML entity escaping. That function also skips the
formatting if the string argument is NULL so no check will be needed.
ACK
I don't see a corresponding change to free this field so it will likely
be leaked.
That was my oversight—I will make sure to free the memory in
virDomainControllerDefFree, and I’ll also check if there are other places where this is
needed.
Since you are using a 'drive' address type here I think this is wrong.
As in you know that if the disk is of VIR_DOMAIN_DISK_BUS_NVME_NS type
it ought to use the 'drive' address type and thus should be categorized
same as SCSI disks.
Yes, you're right.
Please keep the spacing consistent ...
Also the alignment here ..
... and here is broken.
ACK
According to the parser 'serial' seems to be optional. Use of the 's:'
converter makes it mandatory. You likely need 'S:' if it's optional.
The 'serial' field is required—not optional—because the QEMU
implementation mandates its presence.
So what is the story regarding hotplug here? I presume that the
individual namespace devices need to exist before the controller is
attached, right?
In QEMU, the NVMe controller device supports hotplug, but nvme-ns
devices do not, so the current behavior is aligned with QEMU.
Hmm, so if you unplug the controller we'd have to remove all the disks.
Which IMO could work although IIRC the disk unplug code would not be
able to handle this properly yet.
Either way it's okay to forbid libvirt-initiated unplug requests. Also
guest-initiated unplug requests should be complied with and thus the
code for unplug will need to be fixed. Although that is not strictly
required for your series because we have the same kind of issue when
unplugging SCSI controller.
Yes, since nvme-ns devices do not support hot-unplug, controllers with attached
namespaces cannot be hot-unplugged either.
Based on the error message this is not an _INTERNAL_ERROR, but rather
one of those stating that the user messed up with the configuration.
Yes, we should use VIR_ERR_CONFIG_UNSUPPORTED for this case.
I'm not sure I like the 'nvmens' prefix. Let's discuss that in the patch
adding the XML
I agree the nvmens prefix feels a bit odd. Ideally, the naming
should resemble something like nvme0n1, but that would introduce more complexity and
tightly couple the name to a specific controller. As a compromise, maybe we can use nvme
as the prefix.
Also, I’d like to discuss whether nvme-ns is the appropriate choice for the disk bus type.