Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
Hello, This patch is generating errors when booting Libvirt in a Power 8 host: 2020-06-26 21:35:49.703+: 139213: error : virDomainDeviceInfoFormat:7527 : internal error: Missing uid or fid attribute of zPCI address 2020-06-26 21:35:49.703+: 139213: error : virDomainDeviceInfoFormat:7527 : internal error: Missing uid or fid attribute of zPCI address This is not related to Power though. The check for ZPCI address is incomplete is being done prior to the check of ZPCI address is present, in virDomainDeviceInfoFormat(). I already posted a patch that moves the verification to the right code block, avoiding the errors when there is no ZPCI device in the domain: https://www.redhat.com/archives/libvir-list/2020-June/msg01261.html Thanks, DHB
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 [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
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
Andrea Bolognani [2020-06-25, 07:43PM +0200]:
> From 82197ea6d794144e51b72f97df2315a65134b385 Mon Sep 17 00:00:00 2001
> From: Andrea Bolognani
> 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
> ---
> 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);
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.
> +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;
> }
>
> [...]
>
> 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.
Reviewed-by: Bjoern Walk
--
IBM Systems
Linux on Z & Virtualization Development
--
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
signature.asc
Description: PGP signature
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
On 6/25/20 7:43 PM, Andrea Bolognani wrote:
First of all, this is much closer to what I had in mind. Good job!
Hi Andrea,
Thank you:-)
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!
As Boris has already mentioned, these patches provide a much better code,
thank you:-)
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
On 6/25/20 7:43 PM, Andrea Bolognani wrote:
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!
Hi Andrea,
I agree that it looks nice and sparkling. :D Thanks.
Reviewed-by: Boris Fiuczynski
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390
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
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
---
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 (virH
