Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line

On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote: > 在 2018/8/17 上午12:31, Andrea Bolognani 写道: >> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: >>> + virBufferAddLit(&buf, "zpci"); >>> + virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid); >>> + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid); >>> + virBufferAsprintf(&buf, ",target=%s", dev->alias); >>> + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid); >> Just making sure: is the uid a good choice for naming the zpci >> device? I would perhaps expect the fid to be in there as well, >> but since both have to be unique (right?) I guess it doesn't >> really matter... > Either uid or fid, the value must be unique. But uid is defined by user. > Of course, we also give the permission to define fid. But uid is more > appropriate. So which is unique: uid, fid, or the combination of the two? Could I have -device zpci,uid=1,fid=1 -device zpci,uid=1,fid=2 or would that not work? What about -device zpci,uid=1,fid=1 -device zpci,uid=2,fid=1 would that be okay?

>>> +int >>> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info) >> Based on the name, this is a predicate: it should return a >> boolean value rather than an int. > 0 means it's not zpci. 1 means it's zpci. -1 means error. My point is that a funtion called fooIsBaz() should only ever check whether the foo in question is indeed a baz, without taking any other action such as reporting errors. Leave that to the caller, or give the function a different name.

>> Either way, calling a predicate should never result in an error >> being reported, so you need to move this check somewhere else. > Acutally I have found out a proper place to add this check for a long time. > So this is the best place to add, at least I think for now. qemuDomainDeviceDefValidate() looks like a reasonable entry point for checks such as "you specified a zPCI address but this is not an s390 guest" or "your configuration requires zPCI but the QEMU binary doesn't support that feature". Both of the cases above should have proper test suite coverage, by the way.