[PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390

[Andrea Bolognani]

On Fri, 2020-06-26 at 16:08 +0200, Bjoern Walk wrote:
> Andrea Bolognani <abologna at redhat.com> [2020-06-25, 07:43PM +0200]:
> >  static int
> >  virDomainZPCIAddressReserveId(virHashTablePtr set,
> > -                              unsigned int id,
> > +                              virZPCIDeviceAddressID *id,
> >                                const char *name)
> >  {
> > -    if (virHashLookup(set, &id)) {
> > +    if (!id->isSet) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("No zPCI %s to reserve"),
> > +                       name);
> 
> Maybe it's better to fail silently here (or not fail at all and just
> make it no-op)? This is more of an assert that concerns the developer
> and not something the user can understand.

VIR_ERR_INTERNAL_ERROR is commonly used when encountering situations
that Will Never Happen™.

> >  static void
> >  virDomainZPCIAddressReleaseId(virHashTablePtr set,
> > -                              unsigned int *id,
> > +                              virZPCIDeviceAddressID *id,
> >                                const char *name)
> >  {
> > -    if (virHashRemoveEntry(set, id) < 0) {
> > +    if (!id->isSet)
> > +        return;
> > +
> > +    if (virHashRemoveEntry(set, &id->value) < 0) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR,
> >                         _("Release %s %o failed"),
> > -                       name, *id);
> > +                       name, id->value);
> >      }
> >  
> > -    *id = 0;
> > +    id->value = 0;
> > +    id->isSet = false;
> 
> Not sure if I don't want to leave this to the caller. 99% of time it
> shouldn't matter because the released device is deleted from the domain
> anyways, but this tight coupling would prevent hypothetical re-use of
> the id.

id->value is zeroed anyway, so there's not much re-using that can
happen. If you're not deleting the device, then you're going to call
virDomainZPCIAddressAssign{U,F}id() next, and those expect id->isSet
to be false.

-- 
Andrea Bolognani / Red Hat / Virtualization