4:49 p.m.
Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
prior to checking if the device has a ZPCI address at all. This
results in errors like these when starting Libvirt:
error : virDomainDeviceInfoFormat:7527 : internal error:
Missing uid or fid attribute of zPCI address
Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
check done by virZPCIDeviceAddressIsPresent().
Fixes: 076591009ad11ec108521b52a4945d0f895fa160
CC: Bjoern Walk <bwalk(a)linux.ibm.com>
CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
CC: Andrea Bolognani <abologna(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 31ba78b950..33f177b16f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7522,11 +7522,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virTristateSwitchTypeToString(info->addr.pci.multi));
}
- if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing uid or fid attribute of zPCI address"));
- }
if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) {
+ if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci))
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing uid or fid attribute of zPCI
address"));
+
virBufferAsprintf(&childBuf,
"<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
info->addr.pci.zpci.uid.value,
--
2.26.2
8:32 a.m.
On Fri, 2020-06-26 at 18:49 -0300, Daniel Henrique Barboza wrote:
Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
prior to checking if the device has a ZPCI address at all. This
results in errors like these when starting Libvirt:
error : virDomainDeviceInfoFormat:7527 : internal error:
Missing uid or fid attribute of zPCI address
Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
check done by virZPCIDeviceAddressIsPresent().
Fixes: 076591009ad11ec108521b52a4945d0f895fa160
CC: Bjoern Walk <bwalk(a)linux.ibm.com>
CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
CC: Andrea Bolognani <abologna(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Good catch!
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
and pushed with an ever so slightly tweaked commit message.
In the future, please don't include CC tags in your commits: removing
them results in extra work when picking up a patch, and it's also
generally not considered very polite to CC individual developers.
Everyone is subscribed to the list anyway :)
--
Andrea Bolognani / Red Hat / Virtualization
7:25 a.m.
On 6/27/20 10:32 AM, Andrea Bolognani wrote:
On Fri, 2020-06-26 at 18:49 -0300, Daniel Henrique Barboza wrote:
> Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
> s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
> prior to checking if the device has a ZPCI address at all. This
> results in errors like these when starting Libvirt:
>
> error : virDomainDeviceInfoFormat:7527 : internal error:
> Missing uid or fid attribute of zPCI address
>
> Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
> check done by virZPCIDeviceAddressIsPresent().
>
> Fixes: 076591009ad11ec108521b52a4945d0f895fa160
> CC: Bjoern Walk <bwalk(a)linux.ibm.com>
> CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
> CC: Andrea Bolognani <abologna(a)redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> src/conf/domain_conf.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Good catch!
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
and pushed with an ever so slightly tweaked commit message.
In the future, please don't include CC tags in your commits: removing
them results in extra work when picking up a patch, and it's also
generally not considered very polite to CC individual developers.
Everyone is subscribed to the list anyway :)
I screwed up with the CC: tags in the commit msg. I usually do it via
"--cc" in git-sendpatch in these cases.
As for not being polite, I CC'ed the people that was related with the
commit that I was fixing (signed-off/reviewed-by). Perhaps I should
have CC'ed just the author instead .... assuming that CC'ing the author
of the commit I'm amending is OK here in Libvirt, of course. At least
in the QEMU mailing list it's not just OK, but encouraged to CC the
author of the commit you're fixing to make the person aware.
Thanks,
DHB
3:44 a.m.
On Sun, 2020-06-28 at 09:25 -0300, Daniel Henrique Barboza wrote:
On 6/27/20 10:32 AM, Andrea Bolognani wrote:
> In the future, please don't include CC tags in your commits: removing
> them results in extra work when picking up a patch, and it's also
> generally not considered very polite to CC individual developers.
> Everyone is subscribed to the list anyway :)
I screwed up with the CC: tags in the commit msg. I usually do it via
"--cc" in git-sendpatch in these cases.
As for not being polite, I CC'ed the people that was related with the
commit that I was fixing (signed-off/reviewed-by). Perhaps I should
have CC'ed just the author instead .... assuming that CC'ing the author
of the commit I'm amending is OK here in Libvirt, of course. At least
in the QEMU mailing list it's not just OK, but encouraged to CC the
author of the commit you're fixing to make the person aware.
QEMU and libvirt are different projects, which follow different
conventions in many areas such as coding style, merge workflow, and
mailing list usage.
For libvirt specifically, our documentation[1] states
As a rule, patches should be sent to the mailing list only: all
developers are subscribed to libvir-list and read it regularly, so
**please don't CC individual developers** unless they've explicitly
asked you to.
[1] https://libvirt.org/submitting-patches.html
--
Andrea Bolognani / Red Hat / Virtualization
5:47 a.m.
On 6/29/20 5:44 AM, Andrea Bolognani wrote:
On Sun, 2020-06-28 at 09:25 -0300, Daniel Henrique Barboza wrote:
> On 6/27/20 10:32 AM, Andrea Bolognani wrote:
>> In the future, please don't include CC tags in your commits: removing
>> them results in extra work when picking up a patch, and it's also
>> generally not considered very polite to CC individual developers.
>> Everyone is subscribed to the list anyway :)
>
> I screwed up with the CC: tags in the commit msg. I usually do it via
> "--cc" in git-sendpatch in these cases.
>
> As for not being polite, I CC'ed the people that was related with the
> commit that I was fixing (signed-off/reviewed-by). Perhaps I should
> have CC'ed just the author instead .... assuming that CC'ing the author
> of the commit I'm amending is OK here in Libvirt, of course. At least
> in the QEMU mailing list it's not just OK, but encouraged to CC the
> author of the commit you're fixing to make the person aware.
QEMU and libvirt are different projects, which follow different
conventions in many areas such as coding style, merge workflow, and
mailing list usage.
For libvirt specifically, our documentation[1] states
As a rule, patches should be sent to the mailing list only: all
developers are subscribed to libvir-list and read it regularly, so
**please don't CC individual developers** unless they've explicitly
asked you to.
Got it. I suppose this rule will stay the same after we move to Gitlab,
which makes me a bit nervous. Today I can ask people to put me in the CC
if they're fixing/reverting a commit I've authored, but I don't know if
this is possible at all with pull requests.
Thanks,
DHB
7:03 a.m.
On Mon, 2020-06-29 at 07:47 -0300, Daniel Henrique Barboza wrote:
On 6/29/20 5:44 AM, Andrea Bolognani wrote:
> For libvirt specifically, our documentation[1] states
>
> As a rule, patches should be sent to the mailing list only: all
> developers are subscribed to libvir-list and read it regularly, so
> **please don't CC individual developers** unless they've explicitly
> asked you to.
Got it. I suppose this rule will stay the same after we move to Gitlab,
which makes me a bit nervous. Today I can ask people to put me in the CC
if they're fixing/reverting a commit I've authored, but I don't know if
this is possible at all with pull requests.
Assuming you have a GitLab account, tagging you when submitting a MR
is just as easy CCing you when posting a series to the mailing list.
Whether or not that will happen, is still up to the developer...
--
Andrea Bolognani / Red Hat / Virtualization
10:52 a.m.
Hi Daniel,
Sorry for the incorrect code. Thank you for identifying and fixing it:-)
I would like to know, how did you identify the error?,
Is it possible to check for internal errors with the help of unit tests?
Your answer would help me able to identify any incorrect code before
sending the patch.
Warm Regards
Shalini C S
On 6/26/20 11:49 PM, Daniel Henrique Barboza wrote:
Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
prior to checking if the device has a ZPCI address at all. This
results in errors like these when starting Libvirt:
error : virDomainDeviceInfoFormat:7527 : internal error:
Missing uid or fid attribute of zPCI address
Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
check done by virZPCIDeviceAddressIsPresent().
Fixes: 076591009ad11ec108521b52a4945d0f895fa160
CC: Bjoern Walk <bwalk(a)linux.ibm.com>
CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
CC: Andrea Bolognani <abologna(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 31ba78b950..33f177b16f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7522,11 +7522,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
virTristateSwitchTypeToString(info->addr.pci.multi));
}
- if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing uid or fid attribute of zPCI
address"));
- }
if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) {
+ if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci))
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing uid or fid attribute of zPCI
address"));
+
virBufferAsprintf(&childBuf,
"<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
info->addr.pci.zpci.uid.value,
12:44 p.m.
Hi Shalini,
On 6/30/20 12:52 PM, Shalini Chellathurai Saroja wrote:
Hi Daniel,
Sorry for the incorrect code. Thank you for identifying and fixing it:-)
Glad I could help :)
I would like to know, how did you identify the error?,
It was by chance. I rebased the code to master before posting a
series and started libvirtd to do a final test.
I noticed the errors messages right after starting the daemon. What
I did then was verifying which commit added the message and moved it
inside the 'if' block, avoiding checking for zPCI address coherence
regardless of the device having a zPCI address or not.
Is it possible to check for internal errors with the help of unit
tests?
Yes, but this was a peculiar case because the condition in which the error
message was being thrown wasn't causing the function to error out (i.e. returning
-1):
if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing uid or fid attribute of zPCI address"));
}
All existing tests were passing, but the message was being displayed in the log.
This is the kind of situation that, unless you design a test specifically to catch
it, most likely will be noticed when someone runs libvirtd.
Thanks,
DHB
Your answer would help me able to identify any incorrect code before
sending the patch.
Warm Regards
Shalini C S
On 6/26/20 11:49 PM, Daniel Henrique Barboza wrote:
> Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
> s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
> prior to checking if the device has a ZPCI address at all. This
> results in errors like these when starting Libvirt:
>
> error : virDomainDeviceInfoFormat:7527 : internal error:
> Missing uid or fid attribute of zPCI address
>
> Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
> check done by virZPCIDeviceAddressIsPresent().
>
> Fixes: 076591009ad11ec108521b52a4945d0f895fa160
> CC: Bjoern Walk <bwalk(a)linux.ibm.com>
> CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
> CC: Andrea Bolognani <abologna(a)redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> src/conf/domain_conf.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 31ba78b950..33f177b16f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7522,11 +7522,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>
virTristateSwitchTypeToString(info->addr.pci.multi));
> }
> - if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Missing uid or fid attribute of zPCI
address"));
> - }
> if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) {
> + if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci))
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Missing uid or fid attribute of zPCI
address"));
> +
> virBufferAsprintf(&childBuf,
> "<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
> info->addr.pci.zpci.uid.value,
2:35 a.m.
Hi Daniel,
Thank you for letting me know:-)
Warm Regards
Shalini C S
On 6/30/20 7:44 PM, Daniel Henrique Barboza wrote:
Hi Shalini,
On 6/30/20 12:52 PM, Shalini Chellathurai Saroja wrote:
> Hi Daniel,
>
> Sorry for the incorrect code. Thank you for identifying and fixing it:-)
Glad I could help :)
>
> I would like to know, how did you identify the error?,
It was by chance. I rebased the code to master before posting a
series and started libvirtd to do a final test.
I noticed the errors messages right after starting the daemon. What
I did then was verifying which commit added the message and moved it
inside the 'if' block, avoiding checking for zPCI address coherence
regardless of the device having a zPCI address or not.
> Is it possible to check for internal errors with the help of unit tests?
Yes, but this was a peculiar case because the condition in which the
error
message was being thrown wasn't causing the function to error out
(i.e. returning
-1):
if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing uid or fid attribute of zPCI
address"));
}
All existing tests were passing, but the message was being displayed
in the log.
This is the kind of situation that, unless you design a test
specifically to catch
it, most likely will be noticed when someone runs libvirtd.
Thanks,
DHB
> Your answer would help me able to identify any incorrect code before
> sending the patch.
>
> Warm Regards
> Shalini C S
>
>
> On 6/26/20 11:49 PM, Daniel Henrique Barboza wrote:
>> Commit 076591009ad1 ("conf: fix zPCI address auto-generation on
>> s390") is doing a check for virZPCIDeviceAddressIsIncomplete()
>> prior to checking if the device has a ZPCI address at all. This
>> results in errors like these when starting Libvirt:
>>
>> error : virDomainDeviceInfoFormat:7527 : internal error:
>> Missing uid or fid attribute of zPCI address
>>
>> Fix it by moving virZPCIDeviceAddressIsIncomplete() after the
>> check done by virZPCIDeviceAddressIsPresent().
>>
>> Fixes: 076591009ad11ec108521b52a4945d0f895fa160
>> CC: Bjoern Walk <bwalk(a)linux.ibm.com>
>> CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com>
>> CC: Andrea Bolognani <abologna(a)redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
>> ---
>> src/conf/domain_conf.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 31ba78b950..33f177b16f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -7522,11 +7522,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>> virTristateSwitchTypeToString(info->addr.pci.multi));
>> }
>> - if (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci)) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("Missing uid or fid attribute of zPCI
>> address"));
>> - }
>> if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) {
>> + if
>> (virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci))
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Missing uid or fid attribute of
>> zPCI address"));
>> +
>> virBufferAsprintf(&childBuf,
>> "<zpci uid='0x%.4x'
fid='0x%.8x'/>\n",
>> info->addr.pci.zpci.uid.value,
1635
days inactive
1640
days old
8 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel Henrique Barboza -
Shalini Chellathurai Saroja