diff mbox

[1/2] libxl: fix _SC_GETPW_R_SIZE_MAX usage

Message ID 1452604467-65746-2-git-send-email-roger.pau@citrix.com
State New, archived
Headers show

Commit Message

Roger Pau Monne Jan. 12, 2016, 1:14 p.m. UTC
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(+)

Comments

Doug Goldstein Jan. 13, 2016, 3:19 a.m. UTC | #1
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
Roger Pau Monne Jan. 13, 2016, 9:08 a.m. UTC | #2
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.
Ian Campbell Jan. 15, 2016, 10:16 a.m. UTC | #3
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 mbox

Patch

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);