在 2018/8/22 下午5:56, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
在 2018/8/17 上午12:06, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+static inline bool
+virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
+{
+ return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+ info->addr.pci.zpci;
+}
This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.
Got it. Thank!
The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :)
I saw them. Could I give my ACKed?
[...]
+static int
+virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
+ virZPCIDeviceAddressPtr addr)
+{
+ if (!addr->uid_assigned &&
+ virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
+ return -1;
+ }
+
+ if (!addr->fid_assigned &&
+ virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
+ virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+ return -1;
+ }
Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.
If uid/fid is assigned, we call ***Reserve***().
If uid/fid is unassigned, we call ***ReserveNext***().
But I'm not very clear about your concern.
That in ReserveNext() you're checking whether addr->*id_assigned
when you know that ids have not been assigned or you wouldn't have
called ReserveNext() in the first place... It seems unnecessary
and confusing to me, so unless I've missed something that makes it
necessary please just drop those checks.
As our discussion on the 1st patch, boolean values are removed.
So we don't need checks here.
[...]
+static int
+virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
+ virDomainDeviceInfoPtr dev)
+{
It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.
We have to pass DeviceInfo which already has extFlags. If we pass
extFlags again,
isn't it redundant? This function is only called in one place for
hotplug case.
I wanted the API to be more consistent but I realize now you have
to pass either virPCIDeviceAddress or virDomainDeviceInfo depending
on the context, so it doesn't really matter, you can leave it as it
is. The signatures for the corresponding PCI functions are not
entirely consistent either :)
I have a question about your previous comment about error report.
You thought we should report more specific information.
+ virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
+
+ if (zpci && !dev->pciAddressExtFlags) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported."));
+ return -1;
+ }
It's called by all device types which possibly use PCI address.
I'm not sure how to report device's name in error string.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
