Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
On Fri, 2020-06-26 at 16:08 +0200, Bjoern Walk wrote:
Andrea Bolognani <abologna(a)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