On Sat, Dec 15, 2018 at 5:57 PM Rich Felker wrote: > On Sun, Dec 16, 2018 at 02:01:56AM +0200, сергей волковичь wrote: > > cabulertion. > > i finded additional patches for implementing some missing musl features. > This seems to be as if you are trying to port musl to systemd I think Opposite would be a better approach qsort_r probably is a good to consider everything else I am not so sure In oepnembedded we have patched systemd to Undo these things which are being introduced here please take a look > > > > > --- a/include/stdlib.h > > +++ b/include/stdlib.h > > @@ -120,6 +120,9 @@ > > #if defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) \ > > || defined(_BSD_SOURCE) > > char *realpath (const char *__restrict, char *__restrict); > > +#ifdef _GNU_SOURCE > > +char *canonicalize_file_name (const char *__restrict); > > +#endif > > Small nit but rather than adding new #ifdef _GNU_SOURCE sections for > each declaration, any new ones should be added under the existing > section if there is one (and in stdlib.h there is). > > > long int random (void); > > void srandom (unsigned int); > > char *initstate (unsigned int, char *, size_t); > > --- a/src/misc/realpath.c > > +++ b/src/misc/realpath.c > > @@ -42,4 +42,9 @@ > > err: > > __syscall(SYS_close, fd); > > return 0; > > } > > + > > +char *canonicalize_file_name(const char *restrict filename) > > +{ > > + return realpath(filename, NULL); > > +} > > This can't go in the same file as realpath because it's not in the > reserved namespace. Further it's probably not an acceptable addition > since it's not widely used and has no advantages over the standard way > of writing the exact same thing using realpath directly. > > > --- a/include/netdb.h > > +++ b/include/netdb.h > > @@ -124,6 +124,8 @@ > > #define NO_RECOVERY 3 > > #define NO_DATA 4 > > #define NO_ADDRESS NO_DATA > > +#define NETDB_INTERNAL -1 > > +#define NETDB_SUCCESS 0 > > #endif > > > > > > --- a/include/ftw.h > > +++ b/include/ftw.h > > @@ -20,6 +20,14 @@ > > #define FTW_MOUNT 2 > > #define FTW_CHDIR 4 > > #define FTW_DEPTH 8 > > + > > +#ifdef _GNU_SOURCE > > +#define FTW_ACTIONRETVAL 16 > > +#define FTW_CONTINUE 0 > > +#define FTW_STOP 1 > > +#define FTW_SKIP_SUBTREE 2 > > +#define FTW_SKIP_SIBLINGS 3 > > +#endif > > This one and the corresponding functional change is probably worth > consideration. I've seen a few a > > > > > struct FTW { > > int base; > > --- a/src/misc/nftw.c > > +++ b/src/misc/nftw.c > > @@ -1,3 +1,8 @@ > > +#define FTW_ACTIONRETVAL 16 > > +#define FTW_CONTINUE 0 > > +#define FTW_STOP 1 > > +#define FTW_SKIP_SUBTREE 2 > > +#define FTW_SKIP_SIBLINGS 3 > > #include > > #include > > #include > > @@ -58,8 +58,20 @@ > > lev.level = new.level; > > lev.base = h ? h->base : (name=strrchr(path, '/')) ? name-path : 0; > > > > - if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) > > - return r; > > + if (!(flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) { > > + if (flags & FTW_ACTIONRETVAL) > > + switch (r) { > > + case FTW_SKIP_SUBTREE: > > + h = NULL; > > + case FTW_CONTINUE: > > + break; > > + case FTW_SKIP_SIBLINGS: > > + case FTW_STOP: > > + return r; > > + } > > + else > > + return r; > > + } > > > > for (; h; h = h->chain) > > if (h->dev == st.st_dev && h->ino == st.st_ino) > > @@ -82,7 +94,10 @@ > > strcpy(path+j+1, de->d_name); > > if ((r=do_nftw(path, fn, fd_limit-1, > flags, &new))) { > > closedir(d); > > - return r; > > + if ((flags & FTW_ACTIONRETVAL) && > r == FTW_SKIP_SIBLINGS) > > + break; > > + else > > + return r; > > } > > } > > closedir(d); > > @@ -93,8 +108,16 @@ > > } > > > > path[l] = 0; > > - if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) > > - return r; > > + if ((flags & FTW_DEPTH) && (r=fn(path, &st, type, &lev))) { > > + if (flags & FTW_ACTIONRETVAL) > > + switch (r) { > > + case FTW_SKIP_SIBLINGS: > > + case FTW_STOP: > > + return r; > > + } > > + else > > + return r; > > + } > > > > return 0; > > } > > > --- /dev/null > > +++ b/src/misc/parse-printf-format.c > > @@ -0,0 +1,265 @@ > > +/*** > > + Copyright 2014 Emil Renner Berthing > > + With parts from the musl C library > > + Copyright 2005-2014 Rich Felker, et al. > > + > > + This file is free software; you can redistribute it and/or modify it > > + under the terms of the GNU Lesser General Public License as published > by > > + the Free Software Foundation; either version 2.1 of the License, or > > + (at your option) any later version. > > Please do not send license-incompatible patches to musl. > > > --- /dev/null > > +++ b/include/printf.h > > @@ -0,0 +1,41 @@ > > +/*** > > + Copyright 2014 Emil Renner Berthing > > + With parts from the GNU C Library > > + Copyright 1991-2014 Free Software Foundation, Inc. > > + > > + This file is free software; you can redistribute it and/or modify it > > + under the terms of the GNU Lesser General Public License as published > by > > Likewise. > > > + the Free Software Foundation; either version 2.1 of the License, or > > + (at your option) any later version. > > + > > + This file is distributed in the hope that it will be useful, but > > + WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + Lesser General Public License for more details. > > +***/ > > + > > +#pragma once > > #pragma once is not valid C. There is a universal standard way to do this. > > > --- a/include/stdlib.h > > +++ b/include/stdlib.h > > @@ -52,10 +52,13 @@ > > char *getenv (const char *); > > > > int system (const char *); > > > > +typedef int (*__compar_fn_t)(const void *, const void *); > > This is in the internal namespace and it's an error for any > application to be using it. > > > +typedef int (*comparison_fn_t)(const void *, const void *); > > This is presumably a GNU extension (so would need to be in the > appropriate #ifdef block if added) but it probably doesn't meet the > usefulness criterion. Any code using this non-portable construct can > trivially be fixed to be portable. > > > void *bsearch (const void *, const void *, size_t, size_t, int > (*)(const void *, const void *)); > > void qsort (void *, size_t, size_t, int (*)(const void *, const void > *)); > > +void qsort_r (void *, size_t, size_t, int (*)(const void *, const void > *, void *), void *); > > qsort_r is an open issue pending action with the BSDs and the Austin > Group for unifying and standardizing it. Historically some BSDs had an > opposite-argument-order function by the same name which is why musl > has not added it. It might be good to go now; we need to check. > > > --- a/src/stdlib/qsort.c 2017-01-01 04:27:17.000000000 +0100 > > +++ b/src/stdlib/qsort.c 2017-01-29 00:21:27.194887091 +0100 > > @@ -28,7 +28,7 @@ > > #include "atomic.h" > > #define ntz(x) a_ctz_l((x)) > > > > -typedef int (*cmpfun)(const void *, const void *); > > +typedef int (*cmpfun)(const void *, const void *, void *); > > > > static inline int pntz(size_t p[2]) { > > int r = ntz(p[0] - 1); > > @@ -85,7 +85,7 @@ > > p[1] >>= n; > > } > > > > -static void sift(unsigned char *head, size_t width, cmpfun cmp, int > pshift, size_t lp[]) > > +static void sift(unsigned char *head, size_t width, cmpfun cmp, void* > arg, int pshift, size_t lp[]) > > Passing the extra context arg all the way through everything possibly > has significant performance hit for the standard qsort function. This > needs to be measured, and if so, either 2 separate versions need to be > built, or qsort_r needs to be implemented on top of qsort using TLS. > > > [...] > > + > > +void qsort(void *base, size_t nel, size_t width, int (*cmp)(const void > *, const void *)) > > +{ > > + return qsort_r (base, nel, width, (cmpfun)cmp, NULL); > > +} > > This cast and subsequent calling the cmp function with the incorrect > signature has undefined behavior and can't be used. > > > --- a/include/stdlib.h > > +++ b/include/stdlib.h > > @@ -50,7 +50,8 @@ > > int at_quick_exit (void (*) (void)); > > _Noreturn void quick_exit (int); > > > > char *getenv (const char *); > > +char *secure_getenv (const char *); > > Addition of this function is probably ok, but it needs to be under the > proper _GNU_SOURCE test. > > > > > int system (const char *); > > > > --- /dev/null > > +++ b/src/env/secure_getenv.c > > @@ -0,0 +1,8 @@ > > +#define _DEFAULT_SOURCE 1 > > +#include > > +#include > > + > > +char *secure_getenv(const char *name) > > +{ > > + return issetugid() ? NULL : getenv(name); > > +} > > > --- a/include/string.h > > +++ b/include/string.h > > @@ -85,7 +85,21 @@ > > #endif > > > > #ifdef _GNU_SOURCE > > -#define strdupa(x) strcpy(alloca(strlen(x)+1),x) > > +#ifdef __GNUC__ > > +#define strdupa(x) (__extension__({ \ > > + const char* __old = (x); \ > > + size_t __len = strlen(x) + 1; \ > > + char* __new = (char*)alloca(__len); \ > > + (char*)memcpy(__new, __old, __len); \ > > + })) > > +#define strndupa(x,y) (__extension__({ \ > > + const char* __old = (x); \ > > + size_t __len = strnlen(x, (y)); \ > > + char* __new = (char*)alloca(__len + 1); \ > > + __new[__len] = 0; \ > > + (char*)memcpy(__new, __old, __len); \ > > + })) > > +#endif > > This has been discussed before and we have a pending patch somewhere > that does it in another way. I'll see if I can dig it up and comment > based on that. Ultimately stdndupa is a really bad idea, but if it's > purely inline in a header there's not much burden on us by having it. > > > --- a/include/utmpx.h > > +++ b/include/utmpx.h > > @@ -55,6 +55,12 @@ > > #define USER_PROCESS 7 > > #define DEAD_PROCESS 8 > > > > + > > +#ifdef _GNU_SOURCE > > +#define _PATH_UTMPX "/dev/null/utmp" > > +#define _PATH_WTMPX "/dev/null/wtmp" > > +#endif > > + > > #ifdef __cplusplus > > } > > #endif > > I'm not sure if this is a good idea or not. Those paths have been > problematic in the past. > > > define unimplemented macros to 0 so that systemd compiles > > --- a/include/glob.h > > +++ b/include/glob.h > > @@ -22,6 +22,7 @@ > > int glob(const char *__restrict, int, int (*)(const char *, int), > glob_t *__restrict); > > void globfree(glob_t *); > > > > +#define GLOB_BRACE 0 > > This is definitely not acceptable. It advertises the existence of a > feature we don't have, making it impossible for applications to make > correct use of #ifdef to decide whether to use that feature or whether > to use their own replacement for the glob function that provides it. > > If your goal in all this is to build systemd, a better approach is > just using the systemd patches others have already done. systemd > policy is *opposed* to writing portable code, or to making any > promises about what level of GNU extensions they require and will > require in the future. So if we just added a bunch of random stuff to > make systemd work out of the box, a few months or a year from now we'd > find that it doesn't work anymore, and we have to either add whatever > other new glibc thing they're demanding is, or accept that we added a > bunch of junk for nothing. This has been discussed a lot before and > it's not a game musl is willing to play. > > > --- a/include/regex.h > > +++ b/include/regex.h > > @@ -31,6 +31,7 @@ > > > > #define REG_NOTBOL 1 > > #define REG_NOTEOL 2 > > +#define REG_STARTEND 0 > > Likewise this. There has been a request to add it before, and I think > there's a patch somewhere doing just that. The main concern is > long-term cost. The current regex implementation has sufficient > overhead that it doesn't matter, but if we do a better one at some > point in the future it might come back to bite us. I think it's > probably a good idea to add though. > > > #define REG_OK 0 > > #define REG_NOMATCH 1 > > --- a/include/netdb.h > > +++ b/include/netdb.h > > @@ -140,6 +140,8 @@ > > #define EAI_IDN_ENCODE -105 > > #define NI_MAXHOST 255 > > #define NI_MAXSERV 32 > > +#define NI_IDN 0 > > +#define NI_IDN_USE_STD3_ASCII_RULES 0 > > #endif > > The intent is to do IDN without any special flag, in which case > defining it as 0 would possibly be the right thing to do, but at > present it's another case of wrongly advertising a feature that's not > present and breaks application logic. > > Rich >