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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

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.