Re: [libvirt] [PATCH v2 RESEND 07/12] conf: Introduce parser, formatter for uid and fid

On Thu, 2018-07-26 at 15:15 +0800, Yi Min Zhao wrote: > 在 2018/7/24 下午10:25, Andrea Bolognani 写道: >> [...] >>> +static int >>> +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) >>> +{ >>> + if (!zpci->uid_assigned) >>> + return 1; >>> + >>> + if (zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || >>> + zpci->zpci_uid == 0) { >>> + virReportError(VIR_ERR_XML_ERROR, >>> + _("Invalid PCI address uid='0x%x', " >>> + "must be > 0x0 and <= 0x%x"), >>> + zpci->zpci_uid, >>> + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); >>> + return 0; >>> + } >> fid should be validated as well. > FID has been defined in schema. It is impossible to overflow uint32 range. > So...is it required to validate FID here? Mh, fair enough. Not for the schema part (as I mentioned earlier, that's entirely optional so it can't be relied upon when it comes to validating data) but for the 32-bit fid fitting into a 32-bit datatype by definition. Perhaps add a quick comment pointing out why validating fid is not necessary... Additional thoughts: should you check fid_assigned up there as well? Would it be a good idea to use the more specific uint16_t and uint32_t datatypes, and define VIR_DOMAIN_DEVICE_ZPCI_MAX_*ID in terms of the standard UINT*_MAX macros?

[...] >>> + DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW); >> I haven't actually tried that, but you should be able to add the >> test cases to qemuxml2argvtest at the same time as you add them >> here, for consistency's sake. > The qemu cmd generator is introduced in latter patch. > I add the test cases in the corresponding patch. You should be able to add them to the xml2argv test even before you teach libvirt how to generate a QEMU command line for the new feature; then, when you add the missing pieces, it will be very clear from the diff what exactly changed. But as long as you make sure test end up in both xml2argv and xml2xml by the end of the series, it doesn't really matter.