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

[Laine Stump]

On 5/19/21 8:37 AM, Kristina Hanicova wrote:
> 
> 
> On Wed, May 19, 2021 at 9:58 AM Michal Prívozník <mprivozn at redhat.com 
> <mailto:mprivozn at 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 at redhat.com
>     <mailto:khanicov at 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.


> 
>     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...).