On Fri, 2020-06-26 at 16:08 +0200, Bjoern Walk wrote:
> Andrea Bolognani <abologna@xxxxxxxxxx> [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
