[PATCH v2 1/4] Include new arbitrary limits if not already defined

[PATCH v2 1/4] Include new arbitrary limits if not already defined

[Manolo de Medici]

qemu uses the PATH_MAX and IOV_MAX constants extensively
in the code. Define these constants to sensible values ourselves
if the system doesn't define them already.

Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
---
 include/qemu/osdep.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 9a405bed89..9fb6ac5c64 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
 #define TIME_MAX TYPE_MAXIMUM(time_t)
 #endif

+#ifndef PATH_MAX
+#define PATH_MAX 1024
+#endif
+
+#ifndef IOV_MAX
+#define IOV_MAX 1024
+#endif
+
 /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
  * the wrong type. Our replacement isn't usable in preprocessor
  * expressions, but it is sufficient for our needs. */
--
2.43.0

Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined

[Peter Maydell]

On Thu, 18 Jan 2024 at 16:03, Manolo de Medici <manolodemedici@gmail.com> wrote:
>
> qemu uses the PATH_MAX and IOV_MAX constants extensively
> in the code. Define these constants to sensible values ourselves
> if the system doesn't define them already.
>
> Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> ---
>  include/qemu/osdep.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 9a405bed89..9fb6ac5c64 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
>  #define TIME_MAX TYPE_MAXIMUM(time_t)
>  #endif
>
> +#ifndef PATH_MAX
> +#define PATH_MAX 1024
> +#endif
> +
> +#ifndef IOV_MAX
> +#define IOV_MAX 1024
> +#endif
> +
>  /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
>   * the wrong type. Our replacement isn't usable in preprocessor
>   * expressions, but it is sufficient for our needs. */

Ccing some people who know more about portability concerns
than I do...

thanks
-- PMM

Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined

[Daniel P. Berrangé]

On Thu, Jan 18, 2024 at 05:02:23PM +0100, Manolo de Medici wrote:
> qemu uses the PATH_MAX and IOV_MAX constants extensively
> in the code. Define these constants to sensible values ourselves
> if the system doesn't define them already.

Please give details of what platform(s) lack these constants
in the commit message.

Presumably this is a platform that is outside of our normal
support build target list, since we have at least build
coverage for everything mainstream.

> 
> Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> ---
>  include/qemu/osdep.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 9a405bed89..9fb6ac5c64 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
>  #define TIME_MAX TYPE_MAXIMUM(time_t)
>  #endif
> 
> +#ifndef PATH_MAX
> +#define PATH_MAX 1024
> +#endif
> +
> +#ifndef IOV_MAX
> +#define IOV_MAX 1024
> +#endif

If we're going to add this, since we should be removing the
later duplication:

  #define IOV_MAX 1024

in this same file

> +
>  /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
>   * the wrong type. Our replacement isn't usable in preprocessor
>   * expressions, but it is sufficient for our needs. */
> --
> 2.43.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined

[Peter Maydell]

On Mon, 22 Jan 2024 at 17:27, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jan 18, 2024 at 05:02:23PM +0100, Manolo de Medici wrote:
> > qemu uses the PATH_MAX and IOV_MAX constants extensively
> > in the code. Define these constants to sensible values ourselves
> > if the system doesn't define them already.
>
> Please give details of what platform(s) lack these constants
> in the commit message.
>
> Presumably this is a platform that is outside of our normal
> support build target list, since we have at least build
> coverage for everything mainstream.

It's GNU Hurd. The patchset isn't threaded, but the cover
letter is
https://lore.kernel.org/qemu-devel/CAHP40m=UQ=F1-Vy4-wR18RjqzF9o+8UOjgpUsrTU8QXn=7eAeA@mail.gmail.com/

and you can pick up the other patches in it by searching the list.

> >
> > Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> > ---
> >  include/qemu/osdep.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 9a405bed89..9fb6ac5c64 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
> >  #define TIME_MAX TYPE_MAXIMUM(time_t)
> >  #endif
> >
> > +#ifndef PATH_MAX
> > +#define PATH_MAX 1024
> > +#endif
> > +
> > +#ifndef IOV_MAX
> > +#define IOV_MAX 1024
> > +#endif
>
> If we're going to add this, since we should be removing the
> later duplication:
>
>   #define IOV_MAX 1024
>
> in this same file

Mmm, I wondered about that, although in that case it's
"for when the host has no iov implementation at all
and we're rolling our own".

thanks
-- PMM

Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined

[Manolo de Medici]

On Mon, Jan 22, 2024 at 6:40 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 22 Jan 2024 at 17:27, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 18, 2024 at 05:02:23PM +0100, Manolo de Medici wrote:
> > > qemu uses the PATH_MAX and IOV_MAX constants extensively
> > > in the code. Define these constants to sensible values ourselves
> > > if the system doesn't define them already.
> >
> > Please give details of what platform(s) lack these constants
> > in the commit message.
> >
> > Presumably this is a platform that is outside of our normal
> > support build target list, since we have at least build
> > coverage for everything mainstream.
>
> It's GNU Hurd. The patchset isn't threaded, but the cover
> letter is
> https://lore.kernel.org/qemu-devel/CAHP40m=UQ=F1-Vy4-wR18RjqzF9o+8UOjgpUsrTU8QXn=7eAeA@mail.gmail.com/
>
> and you can pick up the other patches in it by searching the list.
>
> > >
> > > Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> > > ---
> > >  include/qemu/osdep.h | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 9a405bed89..9fb6ac5c64 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
> > >  #define TIME_MAX TYPE_MAXIMUM(time_t)
> > >  #endif
> > >
> > > +#ifndef PATH_MAX
> > > +#define PATH_MAX 1024
> > > +#endif
> > > +
> > > +#ifndef IOV_MAX
> > > +#define IOV_MAX 1024
> > > +#endif
> >
> > If we're going to add this, since we should be removing the
> > later duplication:
> >
> >   #define IOV_MAX 1024
> >
> > in this same file
>
> Mmm, I wondered about that, although in that case it's
> "for when the host has no iov implementation at all
> and we're rolling our own".
>
> thanks
> -- PMM

I think that although the two cover different cases, in order for the
patch to be correct we should indeed remove the later duplication,
otherwise compilation will fail on systems lacking IOVEC. Thank you
both for the review

Re: Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined

[Eric Blake]

On Mon, Jan 22, 2024 at 05:19:19PM +0000, Peter Maydell wrote:
> On Thu, 18 Jan 2024 at 16:03, Manolo de Medici <manolodemedici@gmail.com> wrote:
> >
> > qemu uses the PATH_MAX and IOV_MAX constants extensively
> > in the code. Define these constants to sensible values ourselves
> > if the system doesn't define them already.
> >
> > Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> > ---
> >  include/qemu/osdep.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 9a405bed89..9fb6ac5c64 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
> >  #define TIME_MAX TYPE_MAXIMUM(time_t)
> >  #endif
> >
> > +#ifndef PATH_MAX
> > +#define PATH_MAX 1024
> > +#endif

POSIX requires that _XOPEN_PATH_MAX be defined as 1024, as a bare
minimum for any system implementing X/Open extensions to POSIX, so
this number is reasonable.  It is also small enough that most of our
uses where PATH_MAX is used for stack allocation (rather than heap
allocation) don't explode.  But the /reason/ that GNU Hurd refuses to
define PATH_MAX is /because/ it is an arbitrary limit, and GNU Hurd
goes out of its way to not impose such a small limit on the user.  The
intent is that portable code should be written to malloc() any path
operation in order to deal with ANY size file name thrown at the code,
rather than stack-allocate and risk truncation when the limits chosen
were too small for the user's desires.

I'm not opposed to this patch with a stronger commit message, but the
commit message would do well to show that an attempt was made to audit
all existing uses of PATH_MAX and why we still need them rather than
malloc()ing.  A stronger patch would be one that eliminates the use of
PATH_MAX from the code base, on the grounds that truly portable code
can handle all pathnames that the underlying system already has memory
on hand to throw at the program; note that I am /not/ insisting on
such a stronger code guarantee, but merely that we document our design
decision of why we are unable/unwilling to go that far if the stronger
guarantee turns out to be impractical.

> > +
> > +#ifndef IOV_MAX
> > +#define IOV_MAX 1024
> > +#endif

Here, POSIX only requires _XOPEN_IOV_MAX to be 16 (if you're going to
do sharded scatter-gather I/O, portable code has to cater to systems
that do not tolerate you trying to cram more than 16 shards into one
syscall).  Older Solaris actually had a limit this low, but I can
easily test that Linux has a limit of 1024, and a Google search seems
to concur that the various BSDs have also settled on 1024.  GNU Hurd
obviously supports more than 1024, but capping at this number is
reasonable.  Unlike eradicating PATH_MAX from the code base, this is
one that makes more sense to me to have in place; although it would
still be worth documenting that all known systems that qemu targets
(including GNU Hurd, if your intent is to make that such a system)
support 1024 rather than the smaller minimum of 16 that POSIX
mandates.

> > +
> >  /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
> >   * the wrong type. Our replacement isn't usable in preprocessor
> >   * expressions, but it is sufficient for our needs. */
> 
> Ccing some people who know more about portability concerns
> than I do...
> 
> thanks
> -- PMM
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org