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

[Michal Prívozník]

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

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