Re: [libvirt] [PATCH v2 RESEND 07/12] conf: Introduce parser, formatter for uid and fid
在 2018/7/24 下午10:25, Andrea Bolognani 写道:
On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> + <define name="uint32">
> + <choice>
> + <data type="string">
> + <param name="pattern">(0x)?[0-9a-fA-F]{1,8}</param>
> + </data>
> + <data type="int">
This should probably be unignedInt instead of int, but other uint*
types defined in the file also use int so if anything changing all
of them would be the job for a follow-up patch.
OK. I recorded this.
[...]
> + <define name="zpciaddress">
> + <optional>
> + <attribute name="uid">
> + <choice>
> + <data type="string">
> + <param
name="pattern">(0x)?[0-9a-fA-F]{1,4}</param>
> + </data>
> + <data type="unsignedInt">
> + <param name="minInclusive">1</param>
> + <param name="maxInclusive">65535</param>
> + </data>
> + </choice>
> + </attribute>
I don't see why you wouldn't just use the existing uint16 type
here. Is that because uid can't be zero? Making sure that's
actually the case is a job for the PostParse() or Validate()
callback anyway, because schema validation is entirely opt-in
and thus can't be relied upon.
All right.
[...]
> +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?
[...]
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory>219136</memory>
> + <vcpu>1</vcpu>
> + <os>
> + <type arch='s390x' machine='s390-ccw'>hvm</type>
The correct machine type would be s390-ccw-virtio.
There are a bunch of existing test files that incorrectly use
s390-ccw, feel free to clean that up as well ;)
Sure.
[...]
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index bbb995656e..e3282e2b98 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -397,6 +397,8 @@ mymain(void)
> QEMU_CAPS_VIRTIO_SCSI);
> DO_TEST("disk-virtio-scsi-ioeventfd",
> QEMU_CAPS_VIRTIO_SCSI);
> + DO_TEST("disk-virtio-s390-zpci",
> + QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW);
> DO_TEST("disk-scsi-megasas",
> QEMU_CAPS_SCSI_MEGASAS);
> DO_TEST("disk-scsi-mptsas1068",
> @@ -476,6 +478,7 @@ mymain(void)
> DO_TEST("hostdev-usb-address", NONE);
> DO_TEST("hostdev-pci-address", NONE);
> DO_TEST("hostdev-vfio", NONE);
> + 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.