src/conf/domain_conf.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
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 "
VIR_CCW_DEVICE_ADDRESS_FMT),
virMacAddrFormat(&net->mac, mac),
+ NULLSTR(net->info.alias),
net->info.addr.ccw.cssid,
net->info.addr.ccw.ssid,
net->info.addr.ccw.devno);
} else if (PCIAddrSpecified) {
virReportError(VIR_ERR_DEVICE_MISSING,
- _("no device found on " VIR_PCI_DEVICE_ADDRESS_FMT),
+ _("no device matching alias %s found on "
+ VIR_PCI_DEVICE_ADDRESS_FMT),
+ 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 (CCWAddrSpecified) {
virReportError(VIR_ERR_DEVICE_MISSING,
- _("no device found on " VIR_CCW_DEVICE_ADDRESS_FMT),
+ _("no device matching alias %s found on "
+ VIR_CCW_DEVICE_ADDRESS_FMT),
+ NULLSTR(net->info.alias),
net->info.addr.ccw.cssid,
net->info.addr.ccw.ssid,
net->info.addr.ccw.devno);
} else if (MACAddrSpecified) {
virReportError(VIR_ERR_DEVICE_MISSING,
- _("no device matching MAC address %s found"),
- virMacAddrFormat(&net->mac, mac));
+ _("no device matching MAC address %s and alias %s found"),
+ virMacAddrFormat(&net->mac, mac),
+ NULLSTR(net->info.alias));
} else {
virReportError(VIR_ERR_DEVICE_MISSING, "%s",
_("no matching device found"));
--
2.31.1
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.
> VIR_CCW_DEVICE_ADDRESS_FMT),
> virMacAddrFormat(&net->mac, mac),
> + NULLSTR(net->info.alias),
> net->info.addr.ccw.cssid,
> net->info.addr.ccw.ssid,
> net->info.addr.ccw.devno);
> } else if (PCIAddrSpecified) {
> virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device found on " VIR_PCI_DEVICE_ADDRESS_FMT),
> + _("no device matching alias %s found on "
> + VIR_PCI_DEVICE_ADDRESS_FMT),
> + 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 (CCWAddrSpecified) {
> virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device found on " VIR_CCW_DEVICE_ADDRESS_FMT),
> + _("no device matching alias %s found on "
> + VIR_CCW_DEVICE_ADDRESS_FMT),
> + NULLSTR(net->info.alias),
> net->info.addr.ccw.cssid,
> net->info.addr.ccw.ssid,
> net->info.addr.ccw.devno);
> } else if (MACAddrSpecified) {
> virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device matching MAC address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> + _("no device matching MAC address %s and alias %s found"),
> + virMacAddrFormat(&net->mac, mac),
> + NULLSTR(net->info.alias));
> } else {
> virReportError(VIR_ERR_DEVICE_MISSING, "%s",
> _("no matching device found"));
>
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
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
On 5/19/21 8:37 AM, Kristina Hanicova wrote:
>
>
> On Wed, May 19, 2021 at 9:58 AM Michal Prívozník <mprivozn@redhat.com
> <mailto: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
> <https://bugzilla.redhat.com/show_bug.cgi?id=1942367>
> >>
> >> Signed-off-by: Kristina Hanicova <khanicov@redhat.com
> <mailto: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.
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.
>
> 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...).
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@redhat.com
>> <mailto: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
>> <https://bugzilla.redhat.com/show_bug.cgi?id=1942367>
>> >>
>> >> Signed-off-by: Kristina Hanicova <khanicov@redhat.com
>> <mailto: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.
>
> 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
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@redhat.com
>>> <mailto: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
>>> <https://bugzilla.redhat.com/show_bug.cgi?id=1942367>
>>> >>
>>> >> Signed-off-by: Kristina Hanicova <khanicov@redhat.com
>>> <mailto: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.
>>
>> 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
© 2016 - 2024 Red Hat, Inc.