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

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(a)redhat.com&gt; >> --- >> 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