[PATCH] conf: Report alias name of the detached device in error
Kristina Hanicova
khanicov at redhat.com
Thu May 20 10:30:08 UTC 2021
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 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
>
I like Laine's idea to get rid of those long error reports using
"<null>". I will send v2 soon.
Kristina
More information about the libvir-list
mailing list