Messages in this thread | | | From | Zhangjin Wu <> | Subject | Re: [PATCH v5 11/14] tools/nolibc: clean up mmap() support | Date | Mon, 3 Jul 2023 14:51:52 +0800 |
| |
> 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). >
Yeah, we do need such a comment and commit message note, thanks.
Best regards, Zhangjin
> Thanks! > Willy
| |