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

[Julien Grall <julien@xxxxxxx>]

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.
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.

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).
Cheers,

--
Julien Grall