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

[Yi Min Zhao]

在 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.