Re: [PATCH] Check for existence of asprintf and vasprintf
> basename() is in the dirname module That's the POSIX variant. We're using the GNU variant everywhere and the GNU variant is a whopping two lines of code: char *base = strrchr(path, '/'); return base ? base + 1 : (char *)path; > you're correct that GNU strerror_r is not handled by gnulib. > that doesn't look like it's too hard to deal with, but it is > something that'd have to be considered. We're using strerror_r in exactly one place and my proposal is to just return a fixed string if we hit that on platforms where GNU strerror_r is not available. Ulf
Re: [PATCH] Check for existence of asprintf and vasprintf
First, I'm not sure if we want to import the respective gnulib modules directly into the elfutils code base or if you want me to do this in my fork. In the latter case the issue is settled as there is no value for me in jumping through hoops if the code is not going to be upstreamed anyway. So, for now I'm assuming we're talking about importing gnulib modules into the elfutils code base. > gnulib-tool has a --lgpl=[...] flag so you can automatically abort if > the desired license compatibility level isn't met. so you don't have > to directly review every module if it isn't aborting. Are you aware that for most of those modules, building them into elfutils restricts the license choices for the resulting combination? Any non-trivial combination of the required modules with elfutils makes the result de facto GPLv3 only. GPLv3 is fine for me as perfparser is also GPLv3, but as elfutils so far is LGPLv3+/GPLv2+ I'm wondering if we want to do this. In fact, when just doing the usual "configure/make/make install" procedure without reviewing the intermediate results, a user would have no way of knowing what license applies to the binaries. IMO that is bad and will certainly lead to problems somewhere down the line. Ulf
Re: [PATCH] Check for existence of asprintf and vasprintf
Please compare the following code with asprintf.c/vasprintf.c/vasnprintf.c from
gnulib. asprintf depends on vasprintf which in turn depends on vasnprintf
there. vasnprintf is non-standard and not even available on glibc, while
vsnprintf as used by my implementation is standardized by POSIX and widely
available (even on windows). Granted, we have two calls to vsnprintf in my code
vs one call to vasnprintf in the gnulib code. However, if that becomes an
issue, I would rather go for some platform-specific optimization (windows also
has a number of non-standard and possibly useful *printf functions) rather than
importing such a monster.
> +int
> +asprintf(char **strp, const char *fmt, ...)
> +{
> +va_list ap;
> +va_start(ap, fmt);
> +int result = vasprintf(strp, fmt, ap);
> +va_end(ap);
> +return result;
> +}
> +
> +int
> +vasprintf(char **strp, const char *fmt, va_list ap)
> +{
> +va_list test;
> +va_copy(test, ap);
> +int length = vsnprintf(NULL, 0, fmt, test);
> +va_end(test);
> +*strp = malloc(length + 1);
> +if (*strp == NULL)
> +return -1;
> +return vsnprintf(*strp, length + 1, fmt, ap);
> +}
Re: [PATCH] Check for existence of asprintf and vasprintf
On Thu, Feb 23, 2017 at 11:18:53AM +0100, Ulf Hermann wrote: > First, I'm not sure if we want to import the respective gnulib modules > directly into the elfutils code base or if you want me to do this in > my fork. In the latter case the issue is settled as there is no value > for me in jumping through hoops if the code is not going to be > upstreamed anyway. So, for now I'm assuming we're talking about > importing gnulib modules into the elfutils code base. I would be fine with integrating gnulib code upstream. Since we already have a lib and m4 directory it would probably be easiest if we use a new gnulib and gnulib-m4 top-level dir for that. And we'll have to figure out what the best way to handle the sources in the repository is. I think I would prefer option 3 https://www.gnu.org/software/gnulib/manual/html_node/VCS-Issues.html So normal developers don't need to have gnulib itself installed. The release manager would run gnulib-tool --update before release. But maybe there are other ways that are more natural to support? > > gnulib-tool has a --lgpl=[...] flag so you can automatically abort if > > the desired license compatibility level isn't met. so you don't have > > to directly review every module if it isn't aborting. > > Are you aware that for most of those modules, building them into elfutils > restricts the license choices for the resulting combination? Any > non-trivial combination of the required modules with elfutils makes the > result de facto GPLv3 only. GPLv3 is fine for me as perfparser is also > GPLv3, but as elfutils so far is LGPLv3+/GPLv2+ I'm wondering if we want > to do this. In fact, when just doing the usual > "configure/make/make install" procedure without reviewing the > intermediate results, a user would have no way of knowing what license > applies to the binaries. IMO that is bad and will certainly lead to > problems somewhere down the line. As long as everything is upward compatible with GPLv3+ I think that is fine. It will only be an issue on non-GNU/Linux setups. But we should indeed make clear during configure time when extra license requirements would apply because gnulib code is being used/imported. Maybe we can just have configure warn/error out "Your setup requires importing of GPLv3+ gnulib code, please configure with --enable-gplv3-gnulib". Or something like that. Cheers, Mark
Re: [PATCH] Check for existence of mempcpy
On Fri, Feb 17, 2017 at 01:16:11PM +0100, Ulf Hermann wrote: > And then there are nonstandard C constructs, especially void* arithmetics void * arithmetic can at times be surprising, so that seems a fine cleanup. Best to make sure the code compiles cleanly with -Wpointer-arith. > and statement expressions, which I would like to replace with standards > compliant C. That is a somewhat larger and not yet finished project, and > I'm hoping you're generally open to it. Sure, but it depends on the result. Avoiding void * arithmetic seems kind of worth it because it is sometimes surprising and it makes sure we always use the correct type for things involving pointer arithmetic. Which probably cleans up the code. But statement expressions are really useful to define safe macros without having to turn everything into a nested or inlined function. So I am not sure such a specific change would result in cleaner code. So I would have to see the result first. Alternatively we could also just extend the configure test that checks the compiler supports proper GNU99 by adding a statement expression to the testcase (we already expect support for mixed declarations and code, nested functions and arrays of variable lengths because those are really useful). Cheers, Mark
Re: [PATCH] Check for existence of asprintf and vasprintf
On 23 Feb 2017 10:39, Ulf Hermann wrote: > > basename() is in the dirname module > > That's the POSIX variant. We're using the GNU variant everywhere looking closer, it's not either. POSIX doesn't guarantee that the input is not modified, while GNU guarantees that it is left alone. the gnulib one makes the same guarantee as GNU, but it also returns a new buffer. seems like getting a new basename module into gnulib wouldn't be *too* difficult to pull off. > and the GNU variant is a whopping two lines of code: > > char *base = strrchr(path, '/'); > return base ? base + 1 : (char *)path; and we get straight to an example of why your solutions don't scale. your replacement is wrong. and ironically, it's wrong on Windows, which is the whole point of your work. '/' is not the path sep used on every system out there. it also does not properly handle systems (such as Windows) that have filesystem prefixes like C:\. > > you're correct that GNU strerror_r is not handled by gnulib. > > that doesn't look like it's too hard to deal with, but it is > > something that'd have to be considered. > > We're using strerror_r in exactly one place and my proposal is to just return > a fixed string if we hit that on platforms where GNU strerror_r is not > available. i don't have an opinion on that -mike signature.asc Description: Digital signature
