| Message ID | 20220330181947.68497-1-wwcohen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9p: move limits.h include from 9p.c to 9p.h | expand |
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.
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. >
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
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 >
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
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.
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
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 > > >
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
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 > > >
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,
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(-)