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

[Michal Prívozník]

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
>>
>> Signed-off-by: Kristina Hanicova <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.
"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?

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