On Thu, Mar 31, 2022 at 11:34 AM Christian Schoenebeck <
qemu_...@crudebyte.com> wrote:
> 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_...@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 <wwco...@gmail.com> wrote:
> > > > > On Wed, Mar 30, 2022 at 5:31 PM Peter Maydell <
> > >
> > > peter.mayd...@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.
>
Understood. I'll resubmit v2 including all of these changes minus the meson
check.
Thanks,
Will
>
> Best regards,
> Christian Schoenebeck
>
>
>
