| Message ID | 1452604467-65746-2-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/12/16 7:14 AM, Roger Pau Monne wrote: > According to the FreeBSD sysconf man page [0] if the variable is associated > with functionality that is not supported, -1 is returned and errno is not > modified. Modify libxl__dm_runas_helper so it's able to correctly > deal with this situation by setting the initial buffer value to 2048. > > [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > tools/libxl/libxl_dm.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 0aaefd9..ec8fb51 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username) > long buf_size; > int ret; > > + errno = 0; > buf_size = sysconf(_SC_GETPW_R_SIZE_MAX); > + if (buf_size < 0 && errno == 0) { > + buf_size = 2048; > + LOG(DEBUG, > +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld", > + buf_size); > + } > if (buf_size < 0) { > LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld", > buf_size); > So on Linux the behavior is somewhat similar [1]. But I took a peek at the libvirt code for doing the similar thing and I notice that they just default if the value is returned as less than 0 [2]. Reading both man pages it seems like that would be the better bet instead of failing how the current code is. Your fix probably makes it fail less but it could still error out senselessly. Just a suggestion for an improvement overall. [1] http://man7.org/linux/man-pages/man3/sysconf.3.html#RETURN_VALUE [2] http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/virutil.c;h=bb9604a0c1ffb9c99e454e84878a8c376f773046;hb=HEAD#l935
El 13/01/16 a les 4.19, Doug Goldstein ha escrit: > On 1/12/16 7:14 AM, Roger Pau Monne wrote: >> According to the FreeBSD sysconf man page [0] if the variable is associated >> with functionality that is not supported, -1 is returned and errno is not >> modified. Modify libxl__dm_runas_helper so it's able to correctly >> deal with this situation by setting the initial buffer value to 2048. >> >> [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> tools/libxl/libxl_dm.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >> index 0aaefd9..ec8fb51 100644 >> --- a/tools/libxl/libxl_dm.c >> +++ b/tools/libxl/libxl_dm.c >> @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username) >> long buf_size; >> int ret; >> >> + errno = 0; >> buf_size = sysconf(_SC_GETPW_R_SIZE_MAX); >> + if (buf_size < 0 && errno == 0) { >> + buf_size = 2048; >> + LOG(DEBUG, >> +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld", >> + buf_size); >> + } >> if (buf_size < 0) { >> LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld", >> buf_size); >> > > So on Linux the behavior is somewhat similar [1]. But I took a peek at > the libvirt code for doing the similar thing and I notice that they just > default if the value is returned as less than 0 [2]. Reading both man > pages it seems like that would be the better bet instead of failing how > the current code is. Your fix probably makes it fail less but it could > still error out senselessly. Just a suggestion for an improvement overall. Ack, it doesn't make much sense to error out if we cannot find an initial value for the buffer, the code below is able to adapt and expand the buffer as needed anyway. I'm going to resend with that fixed. Thanks, Roger.
On Wed, 2016-01-13 at 10:08 +0100, Roger Pau Monné wrote: > El 13/01/16 a les 4.19, Doug Goldstein ha escrit: > > On 1/12/16 7:14 AM, Roger Pau Monne wrote: > > > According to the FreeBSD sysconf man page [0] if the variable is > > > associated > > > with functionality that is not supported, -1 is returned and errno is > > > not > > > modified. Modify libxl__dm_runas_helper so it's able to correctly > > > deal with this situation by setting the initial buffer value to 2048. > > > > > > [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html (whi ch IMHO is the more canonical documentation) describes a similar error behaviour, although for a subtly different case (no limit for the given variable). > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > --- > > > tools/libxl/libxl_dm.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > > index 0aaefd9..ec8fb51 100644 > > > --- a/tools/libxl/libxl_dm.c > > > +++ b/tools/libxl/libxl_dm.c > > > @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, > > > const char *username) > > > long buf_size; > > > int ret; > > > > > > + errno = 0; > > > buf_size = sysconf(_SC_GETPW_R_SIZE_MAX); > > > + if (buf_size < 0 && errno == 0) { > > > + buf_size = 2048; > > > + LOG(DEBUG, > > > +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld", "... an initial buffer size of ..." > > > + buf_size); > > > + } > > > if (buf_size < 0) { > > > LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error > > > %ld", > > > buf_size); > > > > > > > So on Linux the behavior is somewhat similar [1]. But I took a peek at > > the libvirt code for doing the similar thing and I notice that they > > just > > default if the value is returned as less than 0 [2]. Reading both man > > pages it seems like that would be the better bet instead of failing how > > the current code is. Your fix probably makes it fail less but it could > > still error out senselessly. Just a suggestion for an improvement > > overall. > > Ack, it doesn't make much sense to error out if we cannot find an > initial value for the buffer, the code below is able to adapt and expand > the buffer as needed anyway. I'm going to resend with that fixed. Sounds good. Don't forget to CC all the tools maintainers next time. Ian.
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 0aaefd9..ec8fb51 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username) long buf_size; int ret; + errno = 0; buf_size = sysconf(_SC_GETPW_R_SIZE_MAX); + if (buf_size < 0 && errno == 0) { + buf_size = 2048; + LOG(DEBUG, +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld", + buf_size); + } if (buf_size < 0) { LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld", buf_size);
According to the FreeBSD sysconf man page [0] if the variable is associated with functionality that is not supported, -1 is returned and errno is not modified. Modify libxl__dm_runas_helper so it's able to correctly deal with this situation by setting the initial buffer value to 2048. [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/libxl/libxl_dm.c | 7 +++++++ 1 file changed, 7 insertions(+)