Re: [PATCH for-4.15 v2] xen/dmop: Strip __XEN_TOOLS__ header guard from public API

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 11, 2021 at 01:43:05PM +0000, Andrew Cooper wrote:
> On 11/03/2021 13:28, Jan Beulich wrote:
> > On 11.03.2021 14:00, Andrew Cooper wrote:
> >> However, having laid things out in this way today, it occurs to me that
> >> we should consider further cleanup as well.
> >>
> >> I do agree that code wanting to use the libxendevicemodel.h API almost
> >> certainly don't want/need the dmop ABI.  (i.e. an individual consumer
> >> will want one, or the other, but almost certainly not both together).
> >>
> >> Should libxendevicemodel.h really be including dm_op.h?

FTR, this is xendevicemodel.h. Just saying because it took me a bit to
find the header. I'm dense today.

> > I was indeed wondering.
> >
> >>   AFAICT, it is
> >> only the ioserverid_t typedef which is API shared between the two
> >> contexts, and we can trivially typedef that locally.
> > Hmm, a local typedef isn't nice - there should be one central point.
> > Granted there's no risk for this to change in anywhere halfway
> > foreseeable future, but still. Also neither C89 nor C99 allow a
> > typedef to be repeated - in those versions we'd then rely on an
> > extension.
> 
> I wonder if we're depending on that extension elsewhere.  As far as the
> stable libraries go, we are dependent on a Linux or BSD environment
> currently.
> 
> Alternatively we can drop the typedef and use uint16_t instead without
> breaking things in practice.  (As long as we make the change in 4.15 and
> we lose the wiggle room afforded us by the entire interface being behind
> __XEN_TOOLS__ previously).
> 
> Thoughts?  I can't think of any ifdefary which would help, and swapping
> to uint16_t would reduce the use of an improperly namespaced identifier.

I don't see much problem in switching to uint16_t, it's likely what
should have been used from the start in order to avoid bits of dm_op.h
leaking into xendevicemodel.h. Or alternatively a new type that maps
to uint16_t if we think that would be more descriptive from a header
PoV: server_t or some such.

At the end of day it should be an opaque handler from the caller PoV,
or is it expected that the ioserverid_t obtained from xendevicemodel
will be used as a parameter to other libraries?

If you end up changing the type to uint16_t it might help to expand
the parameter name to server_id or some such, as an id parameter with
type uint16_t is kind of ambiguous. We already have some comment
blocks to describe the purpose of the parameters, so I don't think
it's a big deal if you also leave then as 'id'.

Thanks, Roger.