Re: [PATCH] 9p: move limits.h include from 9p.c to 9p.h

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 31. März 2022 15:19:24 CEST Will Cohen wrote:
> On Thu, Mar 31, 2022 at 7:07 AM Christian Schoenebeck <
> 
> qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 31. März 2022 10:03:35 CEST Peter Maydell wrote:
> > > On Wed, 30 Mar 2022 at 22:55, Will Cohen <wwcohen@gmail.com> wrote:
> > > > On Wed, Mar 30, 2022 at 5:31 PM Peter Maydell <
> > 
> > peter.maydell@linaro.org>
> > 
> > wrote:
> > > >> Is it possible to do this with a meson.build check for whatever
> > > >> host property we're relying on here rather than with a
> > > >> "which OS is this?" ifdef ?
> > > > 
> > > > To confirm -- the game plan in this case would be to do a check for
> > > > something along the lines of
> > > > config_host_data.set('CONFIG_XATTR_SIZE_MAX',
> > > > cc.has_header_symbol('linux/limits.h', 'XATTR_SIZE_MAX')) and using
> > 
> > that
> > 
> > > > in the corresponding ifs, right?
> > > > 
> > > > That makes sense -- if there's no objections, I'll go this route for
> > 
> > v2,
> > 
> > > > which I can submit tomorrow.
> > > 
> > > Yeah, something like that.
> > > 
> > > Looking a bit closer at the code it looks like the handling of
> > > XATTR_SIZE_MAX is kind of odd: on Linux we use this kernel-provided
> > > value, whatever it is, on macos we use a hardcoded 64K, and on
> > > any other host we fail to compile. The comment claims we only
> > > need to impose a limit to avoid doing an overly large malloc,
> > > but if that's the case this shouldn't be OS-specific. I suspect
> > > the problem here is we're trying to impose a non-existent fixed
> > > maximum size for something where the API on the host just doesn't
> > > guarantee one.
> > > 
> > > But that would be a 7.1 thing to look at improving.
> > 
> > It's like this: macOS does not officially have a limit for xattr size in
> > general. HPFS has a xattr size limit on filesystem level it seems up to
> > INT32_MAX, whereas today's APFS's xattr size AFAIK is only limited by the
> > max.
> > APFS file size (8 EB).
> > 
> > As 9p is only used for Linux guests so far, and Linux having a much
> > smaller
> > xattr size limit of 64k, and 9p server still using a very simple RAM only
> > xattr implementation, the idea was to cap the xattr size for macOS hosts
> > to
> > hard coded 64k for that reason for now, at least until there are e.g.
> > macOS 9p
> > guests one day that would then actually start to profit from a streaming
> > xattr
> > implementation in 9p server.
> > 
> > However right now 9p in QEMU only supports Linux hosts and macOS hosts,
> > and
> > the idea of
> > 
> > #else
> > #error Missing definition for P9_XATTR_SIZE_MAX for this host system
> > #endif
> > 
> > was to ensure that whoever adds support for another 9p host system in
> > future,
> > to check what's the limit on that host system, i.e. it might even be <64k.
> > So
> > I wouldn't just blindly use a default value here for all systems.
> 
> Christian, do you have thoughts on the meson.build check, then? For all the
> reasons you state directly above, there's still some macOS-specific logic
> inherent to this functionality. If I create a meson check for
> CONFIG_XATTR_SIZE_MAX, the code becomes something like the following:
> 
> #if defined(CONFIG_XATTR_SIZE_MAX)
> /* Currently, only Linux has XATTR_SIZE_MAX */
> #define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
> #elif defined(CONFIG_DARWIN)
> ...
> 
> On the one hand, I can see how this makes the intent a little clearer --
> there's some kind of conceptual pre-defined header symbol in "most" cases
> (currently only one operating system), with some os-specific fallback logic.
> On the other hand, this isn't really shortening anything, it's just
> replacing CONFIG_LINUX with something which effectively resolves to
> CONFIG_LINUX through redirection.

Well, I don't see an advantage for a meson check in this case, because 
XATTR_SIZE_MAX is a definition that only exists on Linux. Other systems either 
have another macro name or none at all. A dedicated meson check makes sense 
for individual features/macros/symbols that may exist across multiple OSes.

Best regards,
Christian Schoenebeck