On 5/20/21 9:05 AM, Michal Prívozník wrote: > On 5/19/21 7:04 PM, Laine Stump wrote: >> On 5/19/21 8:37 AM, Kristina Hanicova wrote: >>> >>> >>> On Wed, May 19, 2021 at 9:58 AM Michal Prívozník <mprivozn@xxxxxxxxxx >>> <mailto:mprivozn@xxxxxxxxxx>> wrote: >>> >>> On 5/18/21 6:07 PM, Laine Stump wrote: >>> > On 5/18/21 5:44 AM, Kristina Hanicova wrote: >>> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942367 >>> <https://bugzilla.redhat.com/show_bug.cgi?id=1942367> >>> >> >>> >> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx >>> <mailto:khanicov@xxxxxxxxxx>> >>> >> --- >>> >> src/conf/domain_conf.c | 19 +++++++++++++------ >>> >> 1 file changed, 13 insertions(+), 6 deletions(-) >>> >> >>> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> >> index 7044701fac..e21b9fb946 100644 >>> >> --- a/src/conf/domain_conf.c >>> >> +++ b/src/conf/domain_conf.c >>> >> @@ -15781,38 +15781,45 @@ virDomainNetFindIdx(virDomainDef *def, >>> >> virDomainNetDef *net) >>> >> if (matchidx < 0) { >>> >> if (MACAddrSpecified && PCIAddrSpecified) { >>> >> virReportError(VIR_ERR_DEVICE_MISSING, >>> >> - _("no device matching MAC address %s >>> found >>> >> on " >>> >> + _("no device matching MAC address >>> %s and >>> >> alias %s found on " >>> >> VIR_PCI_DEVICE_ADDRESS_FMT), >>> >> virMacAddrFormat(&net->mac, mac), >>> >> + NULLSTR(net->info.alias), >>> >> net->info.addr.pci.domain, >>> >> net->info.addr.pci.bus, >>> >> net->info.addr.pci.slot, >>> >> net->info.addr.pci.function); >>> >> } else if (MACAddrSpecified && CCWAddrSpecified) { >>> >> virReportError(VIR_ERR_DEVICE_MISSING, >>> >> - _("no device matching MAC address %s >>> found >>> >> on " >>> >> + _("no device matching MAC address >>> %s and >>> >> alias %s found on " >>> > >>> > These messages will look strange in the (most common) case where >>> alias >>> > isn't specified, e.g.: >>> > >>> > no device matching MAC address DE:AD:BE:EF:01:10 >>> > and alias found on [some CCW address] >>> > >>> > On the other hand, the idea of even further exploding this >>> bunch of >>> > conditionals to include all combinations is just horrible to >>> think about! >>> > >>> > What about instead reworking this to use a single >>> virReportError() that >>> > references a few pointers setup beforehand and then >>> substituting (a >>> > properly i8n'ized!) "(unspecified)" for each item that hasn't been >>> > specified, e.g.: >>> > >>> > g_autofree *addr = g_strdup(_("(unspecified)")); >>> > const char *mac = _("(unspecified)"); >>> > const char *alias = _("(unspecified)"); >>> > >>> > if (MACAddrSpecified) >>> > mac = virMacAddrFormat(&net->mac, mac); >>> > if (net->info.alias) >>> > alias = net->info.alias >>> > >>> > if (CCWAddrSpecified) >>> > addr = virCCWAddressAsString(blah); >>> > else if (PCIAddrSpecified) >>> > addr = virPCIDeviceAddressAsString(blah); >>> > >>> > virReportError(blah... >>> > _("no device found at address '%s' matching MAC >>> address >>> > '%s' and alias '%s'"), >>> > addr, mac, alias); >>> > >>> > or something like that. It's still not ideal, but avoids the >>> conditional >>> > explosion and I think is less confusing than having "alias" >>> followed by >>> > nothing. >>> >>> IIUC, NULLSTR() will expand to "<null>" not an empty string. >> >> Derp. Oh yeah, you're right! >> >>> "unspecified" sounds better. What I worry about is translations: >>> in my >>> native language and it's not a problem to have the error message >>> split >>> as you suggest. But maybe there are some languages where it might be >>> problem? >> >> I think if it was grammatically a part of the sentence (like the verb or >> something) it would be problematic since the ordering might be wrong >> when translated, but otherwise it should be okay. >> >> Actually having <null> make Kristina's patch seem much less problematic >> to me. It would be nice to use this opportunity to eliminate the big >> chain of different log messages inside if clauses though. >> > > I'm not against that, in fact I like it! We go great lengths to report > what did not match for <interface/>, but not for any other device. > >> >>> >>> On the other hand - we can go with your suggestion and change this >>> later >>> as soon as we learn it's problematic for translators. >>> >>> Kristina, what's your opinion? >>> >>> Michal >>> >>> >>> I think that it can be reworked in a way, that we will have a bool >>> variable for >>> each item that can fail, e.g.: >>> >>> bool aliasMatched = true; >>> bool addrMatched = true; >>> bool macMatched = true; >>> >>> And we would set the corresponding variable to false if they did not >>> match >>> before continuing. When reporting error, we would only report the one >>> last thing >>> it specifically failed on: >>> >>> if (!aliasMatched) >>> virReportError(VIR_ERR_DEVICE_MISSING, >>> _("no device matching alias %s found"), >>> net->info.alias); >>> >>> And so on. >>> But, it might be misleading in case more items did not match. >> >> Yeah, I think this was part of the problem the reporter of the BZ had - >> the log message wasn't giving all the things that were being matched on. >> >>> >>> Maybe we can still go with Laine's suggestion and replace "unspecified" >>> with "<null>" if we worry about translations? >> >> I'm fine with either (assuming that "<null>" is reasonably >> understandable in any language; of course since we already use it in >> other places, I guess that's a pre-existing condition anyway, so...). >> > > Yep, fair enough. Kristina, which version do you like better? > > Michal > I like Laine's idea to get rid of those long error reports using "<null>". I will send v2 soon. Kristina
