Re: [RFC PATCH 12/25] xen: Replace sysctl/readconsole with autogenerated version

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Tue Nov 26, 2024 at 1:20 PM GMT, Jan Beulich wrote:
> On 26.11.2024 13:27, Alejandro Vallejo wrote:
> > On Tue Nov 26, 2024 at 9:40 AM GMT, Jan Beulich wrote:
> >> On 25.11.2024 19:51, Alejandro Vallejo wrote:
> >>> On Mon Nov 25, 2024 at 12:05 PM GMT, Jan Beulich wrote:
> >>>> On 15.11.2024 12:51, Alejandro Vallejo wrote:
> >>>>> Describe sysctl/readconsole as a TOML specification, remove old
> >>>>> hand-coded version and replace it with autogenerated file.
> >>>>>
> >>>>> While at it, transform the console driver to use uint8_t rather than
> >>>>> char in order to mandate the type to be unsigned and ensure the ABI is
> >>>>> not defined with regards to C-specific types.
> >>>>
> >>>> Yet the derived C representation imo then should still be using char, not
> >>>> uint8_t.
> >>>
> >>> There's 2 issued addressed by this patch.
> >>>
> >>>   1. The removal of char from the external headers (and the Xen driver).
> >>>   2. The replacement of the existing struct by the autogenerated one.
> >>>
> >>> (1) wants doing irrespective of (2). char has neither a fixed width nor a 
> >>> fixed
> >>> sign. Which is irrelevant for ABI purposes in this case because what we 
> >>> really
> >>> meant is "give me a pointer" in this hypercall, but it may be important in
> >>> other cases.
> >>>
> >>> IOW, char should've never made it to the definition of the public ABI, 
> >>> and I'm
> >>> merely taking the chance to take it out. Happy to extract this patch and 
> >>> send
> >>> it separately.
> >>
> >> Well, work towards fully getting char out of the public headers may indeed 
> >> be
> >> worthwhile. Otoh with char being the basic addressing granularity, I think
> >> the ABI is pretty much tied to sizeof(char) == 1, imo limiting the
> >> worthwhile-ness quite a bit.
> > 
> > Let me put it another way. If I were to create a separate patch stripping 
> > char
> > and using uint8_t instead, what are my chances of getting an Acked-by? Or 
> > not a
> > NAK, at least. (there's other maintainers that I need that from, but one 
> > step
> > at a time).
>
> That would to some degree depend on what other maintainers think. Not a 
> straight
> NAK in any event.
>
> >> Signed-ness of plain char doesn't really matter as long as it's used only 
> >> for
> >> what really are characters (or strings thereof). And that looks the be 
> >> pretty
> >> much the case throughout the public headers.
> > 
> > Maybe. Still, as a general principle caller and callee ought to agree on 
> > size,
> > alignment and sign for every type. I'd rather not make exceptions for that
> > invariant unless truly well motivated. And in this case it's a case of
> > requiring trivial non-functional changes.
>
> In how far they're non-functional will need to be seen. You also need to keep
> consumers in mind: They may face sudden type disagreement that compilers may
> complain about. Yet "stable" for the public headers means not just the ABI
> itself being stable, but updated headers also being usable as drop-in
> replacements for older versions.

Would it be fair to say that users of the Xen low-level API strictly go via
xenctrl et al? So long as it's only those libraries being affected the effects
should not be externally visible. It's just the hypercall boundary that is
adjusted, not the external API.

>
> >>>> Nit: For anything that is committed, it would be nice if those files 
> >>>> were as
> >>>> tidy as possible style-wise. Most of the above looks entirely okay, just
> >>>> that there is an unnecessary trailing blank line.
> >>>
> >>> I did go out of my way to prettify the output.
> >>>
> >>> The trailing newline was intentional to make it C++03-compatible. I can 
> >>> get rid
> >>> of it, as it doesn't matter a whole lot.
> >>
> >> I haven't followed the development of C++ very closely; my experience with 
> >> it
> >> is mostly from far more than 20 years ago. What's that C++03 compatibility
> >> requirement?
> > 
> > It's really unimportant. -Wnewline-eof, I think it was. There's a pedantic
> > paragraph in the C++ spec from 2003 that states that if a file is not
> > newline-terminated it's UB to parse it. I tend to newline-terminate my files
> > for easy `cat`-ing them together in the rare occasions where that's useful 
> > for
> > something.
>
> Unimportant or not - there must then be some misunderstanding on either my
> side or yours. Even the assembler insists on a final newline, as does plain
> C. Yet that means one newline, not two (i.e. not a trailing blank line).
>
> Jan

Ok, I see what you mean now (only after doing `xxd <file>`). There's indeed an
extra trailing newline. My $EDITOR led me astray.

Cheers,
Alejandro