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

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

[Will Cohen]

As noted by https://gitlab.com/qemu-project/qemu/-/issues/950, within
the patch set adding 9p functionality to darwin, the commit
38d7fd68b0c8775b5253ab84367419621aa032e6 introduced an issue where
limits.h, which defines XATTR_SIZE_MAX, is included in 9p.c, though the
referenced constant is needed in 9p.h. This commit fixes that issue by
moving the include to 9p.h.

Signed-off-by: Will Cohen <wwcohen@gmail.com>
---
 hw/9pfs/9p.c | 5 -----
 hw/9pfs/9p.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index dcaa602d4c..59c531ed47 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -33,11 +33,6 @@
 #include "migration/blocker.h"
 #include "qemu/xxhash.h"
 #include <math.h>
-#ifdef CONFIG_LINUX
-#include <linux/limits.h>
-#else
-#include <limits.h>
-#endif
 
 int open_fd_hw;
 int total_open_fd;
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index af2635fae9..0ce4da375c 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -9,6 +9,11 @@
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
 #include "qemu/qht.h"
+#ifdef CONFIG_LINUX
+#include <linux/limits.h>
+#else
+#include <limits.h>
+#endif
 
 enum {
     P9_TLERROR = 6,
-- 
2.35.1

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

[Philippe Mathieu-Daudé]

Hi,

On 30/3/22 20:19, Will Cohen wrote:
> As noted by https://gitlab.com/qemu-project/qemu/-/issues/950, within
> the patch set adding 9p functionality to darwin, the commit
> 38d7fd68b0c8775b5253ab84367419621aa032e6 introduced an issue where
> limits.h, which defines XATTR_SIZE_MAX, is included in 9p.c, though the
> referenced constant is needed in 9p.h. This commit fixes that issue by
> moving the include to 9p.h.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/950

> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>   hw/9pfs/9p.c | 5 -----
>   hw/9pfs/9p.h | 5 +++++
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index dcaa602d4c..59c531ed47 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -33,11 +33,6 @@
>   #include "migration/blocker.h"
>   #include "qemu/xxhash.h"
>   #include <math.h>
> -#ifdef CONFIG_LINUX
> -#include <linux/limits.h>
> -#else
> -#include <limits.h>
> -#endif
>   
>   int open_fd_hw;
>   int total_open_fd;
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index af2635fae9..0ce4da375c 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -9,6 +9,11 @@
>   #include "qemu/thread.h"
>   #include "qemu/coroutine.h"
>   #include "qemu/qht.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/limits.h>
> +#else
> +#include <limits.h>
> +#endif

Except XATTR_SIZE_MAX, I don't see anything in 9p.h which
requires <limits.h>.

$ git grep P9_XATTR_SIZE_MAX
hw/9pfs/9p.c:3960:    if (size > P9_XATTR_SIZE_MAX) {
hw/9pfs/9p.h:484:#define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
hw/9pfs/9p.h:495:#define P9_XATTR_SIZE_MAX 65536
hw/9pfs/9p.h:497:#error Missing definition for P9_XATTR_SIZE_MAX for 
this host system

Only 9p.c requires this definition, what about moving the
offending code to the .c?

Regards,

Phil.

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

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

On Wed, Mar 30, 2022 at 4:24 PM Philippe Mathieu-Daudé <
philippe.mathieu.daude@gmail.com> wrote:

> Hi,
>
> On 30/3/22 20:19, Will Cohen wrote:
> > As noted by https://gitlab.com/qemu-project/qemu/-/issues/950, within
> > the patch set adding 9p functionality to darwin, the commit
> > 38d7fd68b0c8775b5253ab84367419621aa032e6 introduced an issue where
> > limits.h, which defines XATTR_SIZE_MAX, is included in 9p.c, though the
> > referenced constant is needed in 9p.h. This commit fixes that issue by
> > moving the include to 9p.h.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/950


Thanks -- I'll adjust the syntax accordingly in v2.


>
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >   hw/9pfs/9p.c | 5 -----
> >   hw/9pfs/9p.h | 5 +++++
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index dcaa602d4c..59c531ed47 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -33,11 +33,6 @@
> >   #include "migration/blocker.h"
> >   #include "qemu/xxhash.h"
> >   #include <math.h>
> > -#ifdef CONFIG_LINUX
> > -#include <linux/limits.h>
> > -#else
> > -#include <limits.h>
> > -#endif
> >
> >   int open_fd_hw;
> >   int total_open_fd;
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index af2635fae9..0ce4da375c 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -9,6 +9,11 @@
> >   #include "qemu/thread.h"
> >   #include "qemu/coroutine.h"
> >   #include "qemu/qht.h"
> > +#ifdef CONFIG_LINUX
> > +#include <linux/limits.h>
> > +#else
> > +#include <limits.h>
> > +#endif
>
> Except XATTR_SIZE_MAX, I don't see anything in 9p.h which
> requires <limits.h>.
>
> $ git grep P9_XATTR_SIZE_MAX
> hw/9pfs/9p.c:3960:    if (size > P9_XATTR_SIZE_MAX) {
> hw/9pfs/9p.h:484:#define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
> hw/9pfs/9p.h:495:#define P9_XATTR_SIZE_MAX 65536
> hw/9pfs/9p.h:497:#error Missing definition for P9_XATTR_SIZE_MAX for
> this host system
>
> Only 9p.c requires this definition, what about moving the
> offending code to the .c?
>

That works as well. I suppose I was just trying to keep it conceptually
cleaner with the constants in the .h, but on second thought I agree keeping
it more efficiently contained in the .c is a better move. Will resubmit
with that change as v2.


>
> Regards,
>
> Phil.
>

[-- Attachment #2: Type: text/html, Size: 3639 bytes --]

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

[Christian Schoenebeck]

On Mittwoch, 30. März 2022 23:17:02 CEST Will Cohen wrote:
> On Wed, Mar 30, 2022 at 4:24 PM Philippe Mathieu-Daudé <
> 
> philippe.mathieu.daude@gmail.com> wrote:
> > Hi,
> > 
> > On 30/3/22 20:19, Will Cohen wrote:
> > > As noted by https://gitlab.com/qemu-project/qemu/-/issues/950, within
> > > the patch set adding 9p functionality to darwin, the commit
> > > 38d7fd68b0c8775b5253ab84367419621aa032e6 introduced an issue where
> > > limits.h, which defines XATTR_SIZE_MAX, is included in 9p.c, though the
> > > referenced constant is needed in 9p.h. This commit fixes that issue by
> > > moving the include to 9p.h.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/950
> 
> Thanks -- I'll adjust the syntax accordingly in v2.

As you are sending a v2 anyway, then also add please:

Fixes: 38d7fd68b0 ("9p: darwin: Move XATTR_SIZE_MAX->P9_XATTR_SIZE_MAX")

Also note ...

> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > > 
> > >   hw/9pfs/9p.c | 5 -----
> > >   hw/9pfs/9p.h | 5 +++++
> > >   2 files changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index dcaa602d4c..59c531ed47 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -33,11 +33,6 @@
> > > 
> > >   #include "migration/blocker.h"
> > >   #include "qemu/xxhash.h"
> > >   #include <math.h>
> > > 
> > > -#ifdef CONFIG_LINUX
> > > -#include <linux/limits.h>
> > > -#else
> > > -#include <limits.h>
> > > -#endif
> > > 
> > >   int open_fd_hw;
> > >   int total_open_fd;
> > > 
> > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > > index af2635fae9..0ce4da375c 100644
> > > --- a/hw/9pfs/9p.h
> > > +++ b/hw/9pfs/9p.h
> > > @@ -9,6 +9,11 @@
> > > 
> > >   #include "qemu/thread.h"
> > >   #include "qemu/coroutine.h"
> > >   #include "qemu/qht.h"
> > > 
> > > +#ifdef CONFIG_LINUX
> > > +#include <linux/limits.h>
> > > +#else
> > > +#include <limits.h>
> > > +#endif

... it is usually better to include system headers before project headers.

> > 
> > Except XATTR_SIZE_MAX, I don't see anything in 9p.h which
> > requires <limits.h>.
> > 
> > $ git grep P9_XATTR_SIZE_MAX
> > hw/9pfs/9p.c:3960:    if (size > P9_XATTR_SIZE_MAX) {
> > hw/9pfs/9p.h:484:#define P9_XATTR_SIZE_MAX XATTR_SIZE_MAX
> > hw/9pfs/9p.h:495:#define P9_XATTR_SIZE_MAX 65536
> > hw/9pfs/9p.h:497:#error Missing definition for P9_XATTR_SIZE_MAX for
> > this host system
> > 
> > Only 9p.c requires this definition, what about moving the
> > offending code to the .c?
> 
> That works as well. I suppose I was just trying to keep it conceptually
> cleaner with the constants in the .h, but on second thought I agree keeping
> it more efficiently contained in the .c is a better move. Will resubmit
> with that change as v2.
> 
> > Regards,
> > 
> > Phil.

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

[Peter Maydell]

On Wed, 30 Mar 2022 at 19:26, Will Cohen <wwcohen@gmail.com> wrote:
>
> As noted by https://gitlab.com/qemu-project/qemu/-/issues/950, within
> the patch set adding 9p functionality to darwin, the commit
> 38d7fd68b0c8775b5253ab84367419621aa032e6 introduced an issue where
> limits.h, which defines XATTR_SIZE_MAX, is included in 9p.c, though the
> referenced constant is needed in 9p.h. This commit fixes that issue by
> moving the include to 9p.h.
>
> Signed-off-by: Will Cohen <wwcohen@gmail.com>
> ---
>  hw/9pfs/9p.c | 5 -----
>  hw/9pfs/9p.h | 5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index dcaa602d4c..59c531ed47 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -33,11 +33,6 @@
>  #include "migration/blocker.h"
>  #include "qemu/xxhash.h"
>  #include <math.h>
> -#ifdef CONFIG_LINUX
> -#include <linux/limits.h>
> -#else
> -#include <limits.h>
> -#endif
>
>  int open_fd_hw;
>  int total_open_fd;
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index af2635fae9..0ce4da375c 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -9,6 +9,11 @@
>  #include "qemu/thread.h"
>  #include "qemu/coroutine.h"
>  #include "qemu/qht.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/limits.h>
> +#else
> +#include <limits.h>
> +#endif

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 ?

thanks
-- PMM

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

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

On Wed, Mar 30, 2022 at 5:31 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Wed, 30 Mar 2022 at 19:26, Will Cohen <wwcohen@gmail.com> wrote:
> >
> > As noted by https://gitlab.com/qemu-project/qemu/-/issues/950, within
> > the patch set adding 9p functionality to darwin, the commit
> > 38d7fd68b0c8775b5253ab84367419621aa032e6 introduced an issue where
> > limits.h, which defines XATTR_SIZE_MAX, is included in 9p.c, though the
> > referenced constant is needed in 9p.h. This commit fixes that issue by
> > moving the include to 9p.h.
> >
> > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > ---
> >  hw/9pfs/9p.c | 5 -----
> >  hw/9pfs/9p.h | 5 +++++
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index dcaa602d4c..59c531ed47 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -33,11 +33,6 @@
> >  #include "migration/blocker.h"
> >  #include "qemu/xxhash.h"
> >  #include <math.h>
> > -#ifdef CONFIG_LINUX
> > -#include <linux/limits.h>
> > -#else
> > -#include <limits.h>
> > -#endif
> >
> >  int open_fd_hw;
> >  int total_open_fd;
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index af2635fae9..0ce4da375c 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -9,6 +9,11 @@
> >  #include "qemu/thread.h"
> >  #include "qemu/coroutine.h"
> >  #include "qemu/qht.h"
> > +#ifdef CONFIG_LINUX
> > +#include <linux/limits.h>
> > +#else
> > +#include <limits.h>
> > +#endif
>
> 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.


> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 3122 bytes --]

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

[Peter Maydell]

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.

-- PMM

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

[Christian Schoenebeck]

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.

Best regards,
Christian Schoenebeck

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

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 3655 bytes --]

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.

Will


>
> Best regards,
> Christian Schoenebeck
>
>
>

[-- Attachment #2: Type: text/html, Size: 4687 bytes --]

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

[Christian Schoenebeck]

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

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

[Will Cohen]

[-- Attachment #1: Type: text/plain, Size: 4641 bytes --]

On Thu, Mar 31, 2022 at 11:34 AM Christian Schoenebeck <
qemu_oss@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_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.
>

Understood. I'll resubmit v2 including all of these changes minus the meson
check.

Thanks,
Will


>
> Best regards,
> Christian Schoenebeck
>
>
>

[-- Attachment #2: Type: text/html, Size: 6218 bytes --]