[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

在 2018/8/20 下午7:14, Andrea Bolognani 写道:
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
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?
Both can't work. FID must be unique and UID must be also unique.
They are independent.

+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.
I think moving to the caller is proper.

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.

How about my idea mentioned above?

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]