On Wed, Apr 12, 2023 at 12:37:39PM +0200, Jiri Denemark wrote:
> On Thu, Apr 06, 2023 at 02:20:31 -0700, Andrea Bolognani wrote:
> > On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
> > > On 4/5/23 19:21, Andrea Bolognani wrote:
> > > > On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
> > > >> + if (nodeHeader->len < sizeof(*nodeHeader)) {
> > > >> + virReportError(VIR_ERR_INTERNAL_ERROR,
> > > >> + _("IORT table node type %1$s has invalid length: %2$" PRIu16),
> > > >
> > > > I'm not sure this plays well with the recently introduced changes to
> > > > translatable strings. Have you checked with Jirka?
> > >
> > > I haven't. But, gcc complains if only a part of string contains
> > > positioned arguments. That is:
> > >
> > > "argument value %1$s and another argument %s"
> > > "argument value %s and another argument %2$s"
> > >
> > > are invalid format string and compiler throws an error. Now, using no
> > > positions works:
> > >
> > > "argument value %s and another argument %s"
> > >
> > > but then, Jirka's syntax-check rule throws an error. But it doesn't for
> > > the case I'm using:
> > >
> > > "integer value %" PRIu16
> >
> > Yeah, either your usage is fine or the syntax-check rule should be
> > improve to catch it. Jirka?
>
> Yeah, the syntax check is quite simple so it doesn't catch all possible
> cases and libvirt-pot-check run after libvirt-pot (either in our CI or
> manually) will detect all cases.
Running libvirt-pot updates po/libvirt.pot as such:
-"IORT table node type %1$s has invalid length: got %2$u, expected at least "
-"%3$zu"
+"IORT table node type %1$s has invalid length: got %2$<PRIu16>, expected at "
+"least %3$zu"
Running libvirt-pot-check afterwards doesn't result in a failure. So,
if this is something that we don't want in our pot file, we need to
find a way to detect and report it.
Note that changing the message as described above causes it to trip
up the libvirt_unmarked_diagnostics syntax-check rule. This was not
the case in Michal's original version of the patch, where PRIu16 was
used at the very end of the format string instead of in the middle of
it.
> So the best thing to do here is to avoid the situation and do what you
> came up with here:
>
> > > I can typecast arguments. IOW:
> > >
> > > virReportError(VIR_ERR_INTERNAL_ERROR,
> > > _("IORT table node type %1$s has invalid length: %2$u"),
> > > NULLSTR(typeStr), (unsigned int)nodeHeader->len);
Right, but ideally we wouldn't need to depend on humans noticing :)
To be honest, I'm still unclear on whether allowing this in would
actually have been a problem. Jirka, can you please explicitly
confirm one way or the other?
--
Andrea Bolognani / Red Hat / Virtualization
