Re: [PATCH v5 11/14] tools/nolibc: clean up mmap() support

[Willy Tarreau <w@1wt.eu>]

On Wed, Jun 28, 2023 at 09:41:13PM +0800, Zhangjin Wu wrote:
>  static __attribute__((unused))
>  void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset)
>  {
> -	void *ret = sys_mmap(addr, length, prot, flags, fd, offset);
> -
> -	if ((unsigned long)ret >= -4095UL) {
> -		SET_ERRNO(-(long)ret);
> -		ret = MAP_FAILED;
> -	}
> -	return ret;
> +	return (void *)__sysret((unsigned long)sys_mmap(addr, length, prot, flags, fd, offset));
>  }

One point regarding this one. By doing so, we're hard-coding the fact
that we consider that MAP_FAILED is always -1. I'm not necessarily
against it, but this implication can be confusing for those searching
where it's being set. I would suggest putting a comment before the
mmap() function saying:

 /* Note that on Linux MAP_FAILED is -1 so we can use the generic __sysret()
  * which returns -1 upon error and still satisfy user land that checks for
  * MAP_FAILED.
  */

Since it's an assumed choice that theoretically could affect portability,
it should be reflected in the commit message as well (and we all know it
does not have any impact).

Thanks!
Willy