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 :)
> > [...]
> > > +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.
> > [...]
> > > +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 :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
