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