<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 19, 2021 at 9:58 AM Michal Prívozník <<a href="mailto:mprivozn@redhat.com">mprivozn@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 5/18/21 6:07 PM, Laine Stump wrote:<br>
> On 5/18/21 5:44 AM, Kristina Hanicova wrote:<br>
>> Resolves: <a href="https://bugzilla.redhat.com/show_bug.cgi?id=1942367" rel="noreferrer" target="_blank">https://bugzilla.redhat.com/show_bug.cgi?id=1942367</a><br>
>><br>
>> Signed-off-by: Kristina Hanicova <<a href="mailto:khanicov@redhat.com" target="_blank">khanicov@redhat.com</a>><br>
>> ---<br>
>>   src/conf/domain_conf.c | 19 +++++++++++++------<br>
>>   1 file changed, 13 insertions(+), 6 deletions(-)<br>
>><br>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c<br>
>> index 7044701fac..e21b9fb946 100644<br>
>> --- a/src/conf/domain_conf.c<br>
>> +++ b/src/conf/domain_conf.c<br>
>> @@ -15781,38 +15781,45 @@ virDomainNetFindIdx(virDomainDef *def,<br>
>> virDomainNetDef *net)<br>
>>       if (matchidx < 0) {<br>
>>           if (MACAddrSpecified && PCIAddrSpecified) {<br>
>>               virReportError(VIR_ERR_DEVICE_MISSING,<br>
>> -                           _("no device matching MAC address %s found<br>
>> on "<br>
>> +                           _("no device matching MAC address %s and<br>
>> alias %s found on "<br>
>>                                VIR_PCI_DEVICE_ADDRESS_FMT),<br>
>>                              virMacAddrFormat(&net->mac, mac),<br>
>> +                           NULLSTR(net->info.alias),<br>
>>                              net->info.addr.pci.domain,<br>
>>                              net->info.addr.pci.bus,<br>
>>                              net->info.addr.pci.slot,<br>
>>                              net->info.addr.pci.function);<br>
>>           } else if (MACAddrSpecified && CCWAddrSpecified) {<br>
>>               virReportError(VIR_ERR_DEVICE_MISSING,<br>
>> -                           _("no device matching MAC address %s found<br>
>> on "<br>
>> +                           _("no device matching MAC address %s and<br>
>> alias %s  found on "<br>
> <br>
> These messages will look strange in the (most common) case where alias<br>
> isn't specified, e.g.:<br>
> <br>
>     no device matching MAC address DE:AD:BE:EF:01:10<br>
>     and alias   found on [some CCW address]<br>
> <br>
> On the other hand, the idea of even further exploding this bunch of<br>
> conditionals to include all combinations is just horrible to think about!<br>
> <br>
> What about instead reworking this to use a single virReportError() that<br>
> references a few pointers setup beforehand and then substituting (a<br>
> properly i8n'ized!) "(unspecified)" for each item that hasn't been<br>
> specified, e.g.:<br>
> <br>
> g_autofree *addr = g_strdup(_("(unspecified)"));<br>
> const char *mac = _("(unspecified)");<br>
> const char *alias = _("(unspecified)");<br>
> <br>
> if (MACAddrSpecified)<br>
>     mac = virMacAddrFormat(&net->mac, mac);<br>
> if (net->info.alias)<br>
>     alias = net->info.alias<br>
> <br>
> if (CCWAddrSpecified)<br>
>    addr = virCCWAddressAsString(blah);<br>
> else if (PCIAddrSpecified)<br>
>    addr = virPCIDeviceAddressAsString(blah);<br>
> <br>
> virReportError(blah...<br>
>                _("no device found at address '%s' matching MAC address<br>
> '%s' and alias '%s'"),<br>
>                addr, mac, alias);<br>
> <br>
> or something like that. It's still not ideal, but avoids the conditional<br>
> explosion and I think is less confusing than having "alias" followed by<br>
> nothing.<br>
<br>
IIUC, NULLSTR() will expand to "<null>" not an empty string.<br>
"unspecified" sounds better. What I worry about is translations: in my<br>
native language and it's not a problem to have the error message split<br>
as you suggest. But maybe there are some languages where it might be<br>
problem?<br>
<br>
On the other hand - we can go with your suggestion and change this later<br>
as soon as we learn it's problematic for translators.<br>
<br>
Kristina, what's your opinion?<br>
<br>
Michal<br>
<br></blockquote><div><br></div><div>I think that it can be reworked in a way, that we will have a bool variable for<br>each item that can fail, e.g.:<br><br>bool aliasMatched = true;<br>bool addrMatched = true;<br>bool macMatched = true;<br><br>And we would set the corresponding variable to false if they did not match<br>before continuing. When reporting error, we would only report the one last thing<br>it specifically failed on:<br><br>if (!aliasMatched)<br>    virReportError(VIR_ERR_DEVICE_MISSING,<br>                   _("no device matching alias %s found"),<br>                   net->info.alias);<br><br><div>And so on.</div><div>But, it might be misleading in case more items did not match.</div><br>Maybe we can still go with Laine's suggestion and replace "unspecified"<br><div>with "<null>" if we worry about translations?</div><div><br></div><div>Kristína</div></div><div> </div></div></div>