Re: [PATCH] conf: Report alias name of the detached device in error

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(a)redhat.com >> <mailto:mprivozn@redhat.com>> 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(a)redhat.com >> <mailto:khanicov@redhat.com>> >> >> --- >> >> 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