On Wed, May 19, 2021 at 9:58 AM Michal Prívozník <mprivozn@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
Signed-off-by: Kristina Hanicova <khanicov@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
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. Maybe we can still go with Laine's suggestion and replace "unspecified" with "<null>" if we worry about translations? Kristína