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.
