Re: [PATCH v2 1/2] public/io: xs_wire: Document that EINVAL should always be first in xsd_errors

[Juergen Gross <jgross@xxxxxxxx>]

On 27.06.22 17:03, Julien Grall wrote:
> Hi Juergen,
>
> On 27/06/2022 15:50, Juergen Gross wrote:
> > On 27.06.22 16:48, Julien Grall wrote:
> > > Hi,
> > >
> > > On 27/06/2022 15:31, Juergen Gross wrote:
> > > > On 27.06.22 14:36, Julien Grall wrote:
> > > > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > > >
> > > > > Some tools (e.g. xenstored) always expect EINVAL to be first in xsd_errors.
> > > > >
> > > > > Document it so, one doesn't add a new entry before hand by mistake.
> > > > >
> > > > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > > >
> > > > > ----
> > > > >
> > > > > I have tried to add a BUILD_BUG_ON() but GCC complained that the value
> > > > > was not a constant. I couldn't figure out a way to make GCC happy.
> > > > >
> > > > > Changes in v2:
> > > > >      - New patch
> > > > > ---
> > > > >   xen/include/public/io/xs_wire.h | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
> > > > > index c1ec7c73e3b1..dd4c9c9b972d 100644
> > > > > --- a/xen/include/public/io/xs_wire.h
> > > > > +++ b/xen/include/public/io/xs_wire.h
> > > > > @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
> > > > >   __attribute__((unused))
> > > > >   #endif
> > > > >       = {
> > > > > +    /* /!\ Some users (e.g. xenstored) expect EINVAL to be the first 
> > > > > entry. */
> > > > >       XSD_ERROR(EINVAL),
> > > > >       XSD_ERROR(EACCES),
> > > > >       XSD_ERROR(EEXIST),
> > > > What about another approach, like:
> > > In place of what? I still think we need the comment because this assumption 
> > > is not part of the ABI (AFAICT xs_wire.h is meant to be stable).
> > > At which point, I see limited reason to fix xenstored_core.c.
> > >
> > > But I would have really prefer to use a BUILD_BUG_ON() (or similar) so we can 
> > > catch any misue a build. Maybe I should write a small program that is 
> > > executed at compile time?
> > My suggestion removes the need for EINVAL being the first entry
> xsd_errors[] is part of the stable ABI. If Xenstored is already "misusing" it, 
> then I wouldn't be surprised if other software rely on this as well.
Xenstored is the only instance which needs a translation from value to
string, while all other users should need only the opposite direction.
The only other candidate would be oxenstored, but that seems not to use
xsd_errors[].

And in fact libxenstore will just return a plain EINVAL in case it
can't find a translation, while hvmloader will return EIO in that case.

With your reasoning and the hvmloader use case you could argue that
the EIO entry needs to stay at the same position, too.

> Therefore, I don't really see how fixing Xenstored would allow us to remove this 
> restriction.
> The only reason this was spotted is by Jan reviewing C Xenstored. Without that, 
> it would have problably took a long time to notice
> this change (I don't think there are many other errno used by Xenstored
> and xsd_errors). So I think the risk is not worth the effort.
I don't see a real risk here, but if there is consensus that the risk should
not be taken, then I'd rather add a comment that new entries are only allowed
to be added at the end of the array.

> At least, this is not a patch I would be willing to have my name on (either as a 
> signed-off-by or acked-by).
Fair enough. :-)


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature