On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> @@ -129,7 +129,8 @@ static void
> virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> virZPCIDeviceAddressPtr addr)
> {
> - virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
> + if (addr->uid.isSet)
> + virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid");
> }
>
> @@ -137,7 +138,8 @@ static void
> virDomainZPCIAddressReleaseFid(virHashTablePtr set,
> virZPCIDeviceAddressPtr addr)
> {
> - virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
> + if (addr->fid.isSet)
> + virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid");
> }
>
> -static int
> -virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> - virZPCIDeviceAddressPtr addr)
> -{
> - if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> - return -1;
> -
> - if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
> - virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> - return -1;
> - }
> + addr->uid.isSet = true;
> + addr->fid.isSet = true;
First of all, this is much closer to what I had in mind. Good job!
We're not quite there yet, though: as you can see from the hunks
above, there are still many scenarios in which we're either
manipulating id->value and id->isSet separately, or we needlessly
duplicate checks and don't take advantage of the fact that we now
have the virZPCIDeviceAddressID struct.
I have attached a patch that fixes these issues: as you can see,
it's pretty simple, because you've laid all the groundwork :) so it
just needs to polish things up a bit. It also solves the situation
where calling virDomainZPCIAddressRelease{U,F}id() would not set
id->isSet back to false.
Speaking of polish, while functionally this doesn't make any
difference it would be IMHO better to rename the current
virDomainZPCIAddressReserveAddr() to
virDomainZPCIAddressEnsureAddr(), since in terms of semantics it
more closely matches those of the PCI address function of the same
name. This will hopefully help reduce confusion. I've attached a
patch that performs this change as well.
Everything else looks good. Please let me know what you think of
the changes in the attached patches!
--
Andrea Bolognani / Red Hat / Virtualization
From 82197ea6d794144e51b72f97df2315a65134b385 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@xxxxxxxxxx>
Date: Thu, 25 Jun 2020 18:37:27 +0200
Subject: [libvirt PATCH 1/2] conf: Move id->{value,isSet} handling further
down the stack
Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
---
src/conf/domain_addr.c | 61 ++++++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 90ed31ef4e..493b155129 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -33,20 +33,27 @@ VIR_LOG_INIT("conf.domain_addr");
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);
+ return -1;
+ }
+
+ if (virHashLookup(set, &id->value)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("zPCI %s %o is already reserved"),
- name, id);
+ name, id->value);
return -1;
}
- if (virHashAddEntry(set, &id, (void*)1) < 0) {
+ if (virHashAddEntry(set, &id->value, (void*)1) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to reserve %s %o"),
- name, id);
+ name, id->value);
return -1;
}
@@ -58,7 +65,7 @@ static int
virDomainZPCIAddressReserveUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressReserveId(set, addr->uid.value, "uid");
+ return virDomainZPCIAddressReserveId(set, &addr->uid, "uid");
}
@@ -66,17 +73,20 @@ static int
virDomainZPCIAddressReserveFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressReserveId(set, addr->fid.value, "fid");
+ return virDomainZPCIAddressReserveId(set, &addr->fid, "fid");
}
static int
virDomainZPCIAddressAssignId(virHashTablePtr set,
- unsigned int *id,
+ virZPCIDeviceAddressID *id,
unsigned int min,
unsigned int max,
const char *name)
{
+ if (id->isSet)
+ return 0;
+
while (virHashLookup(set, &min)) {
if (min == max) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -86,7 +96,9 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
}
++min;
}
- *id = min;
+
+ id->value = min;
+ id->isSet = true;
return 0;
}
@@ -96,7 +108,7 @@ static int
virDomainZPCIAddressAssignUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressAssignId(set, &addr->uid.value, 1,
+ return virDomainZPCIAddressAssignId(set, &addr->uid, 1,
VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid");
}
@@ -105,23 +117,27 @@ static int
virDomainZPCIAddressAssignFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- return virDomainZPCIAddressAssignId(set, &addr->fid.value, 0,
+ return virDomainZPCIAddressAssignId(set, &addr->fid, 0,
VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid");
}
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;
}
@@ -129,8 +145,7 @@ static void
virDomainZPCIAddressReleaseUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- if (addr->uid.isSet)
- virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid");
+ virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
}
@@ -138,8 +153,7 @@ static void
virDomainZPCIAddressReleaseFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
{
- if (addr->fid.isSet)
- virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid");
+ virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
}
@@ -159,12 +173,10 @@ static int
virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
- if (!addr->uid.isSet &&
- virDomainZPCIAddressAssignUid(zpciIds->uids, addr) < 0)
+ if (virDomainZPCIAddressAssignUid(zpciIds->uids, addr) < 0)
return -1;
- if (!addr->fid.isSet &&
- virDomainZPCIAddressAssignFid(zpciIds->fids, addr) < 0)
+ if (virDomainZPCIAddressAssignFid(zpciIds->fids, addr) < 0)
return -1;
if (virDomainZPCIAddressReserveUid(zpciIds->uids, addr) < 0)
@@ -175,9 +187,6 @@ virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
return -1;
}
- addr->uid.isSet = true;
- addr->fid.isSet = true;
-
return 0;
}
--
2.25.4
From 9a4653ffaefdce2faec833559fbed022c8c4a708 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@xxxxxxxxxx>
Date: Thu, 25 Jun 2020 19:29:30 +0200
Subject: [libvirt PATCH 2/2] conf: Rename virDomainZPCIAddress{Reserve ->
Ensure}Addr()
Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
---
src/conf/domain_addr.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 493b155129..2f9ff899d7 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -170,7 +170,7 @@ virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
static int
-virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds,
+virDomainZPCIAddressEnsureAddr(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
{
if (virDomainZPCIAddressAssignUid(zpciIds->uids, addr) < 0)
@@ -199,7 +199,7 @@ virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
/* Reserve uid/fid to ZPCI device which has defined uid/fid
* in the domain.
*/
- return virDomainZPCIAddressReserveAddr(addrs->zpciIds, &addr->zpci);
+ return virDomainZPCIAddressEnsureAddr(addrs->zpciIds, &addr->zpci);
}
return 0;
@@ -213,7 +213,7 @@ virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
virZPCIDeviceAddress zpci = addr->zpci;
- if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, &zpci) < 0)
+ if (virDomainZPCIAddressEnsureAddr(addrs->zpciIds, &zpci) < 0)
return -1;
if (!addrs->dryRun)
@@ -231,7 +231,7 @@ virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
if (addr->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
virZPCIDeviceAddressPtr zpci = &addr->zpci;
- if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci) < 0)
+ if (virDomainZPCIAddressEnsureAddr(addrs->zpciIds, zpci) < 0)
return -1;
}
--
2.25.4
