Re: [PATCH 1/1] domain_conf.c: skip checking ZPCI address is incomplete if not present

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&gt; >> CC: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; >> CC: Shalini Chellathurai Saroja <shalini(a)linux.ibm.com&gt; >> CC: Andrea Bolognani <abologna(a)redhat.com&gt; >> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; >> --- >>   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,