Libguestfs - Jul 2016 - Re: [PATCH] mllib: Getopt: fix integer parsing

On Monday, 18 July 2016 09:38:43 CEST Richard W.M. Jones
wrote:
> On Mon, Jul 18, 2016 at 10:13:30AM +0200, Pino Toscano wrote:
> > Since we are using gnulib already, make use of xstrtol to parse the
> > integer arguments to avoid extra suffixes, etc.
> > 
> > Fixes commit 0f7bf8f714898c606e5d5015fff5b7803dcd1aee.
> > ---
> >  mllib/getopt-c.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/mllib/getopt-c.c b/mllib/getopt-c.c
> > index 1f129a7..2d3f9b6 100644
> > --- a/mllib/getopt-c.c
> > +++ b/mllib/getopt-c.c
> > @@ -30,6 +30,8 @@
> >  #include <error.h>
> >  #include <assert.h>
> >  
> > +#include "xstrtol.h"
> > +
> >  #include <caml/alloc.h>
> >  #include <caml/fail.h>
> >  #include <caml/memory.h>
> > @@ -117,6 +119,26 @@ do_call1 (value funv, value paramv)
> >    CAMLreturn0;
> >  }
> 
> This function needs to return something other than 'int', since on
64
> bit OCaml integers (the final destination) are signed 63 bits.  I
> think returning 'long' is a better idea, and the receiving
'num'
> should also be 'long' (as in my patch).
I still don't understand why we need to handle values bigger than int
(as in C int, i.e. signed 32 bits) at all -- neither it is actually
needed, nor it would be coherent in 32bit vs 64bit builds.

If we really 64bit values as parameters, then let's just create a new
option type marking that.
> 
> > +static int
> > +strtoint (const char *arg)
> > +{
> > +  long int num;
> > +
> > +  if (xstrtol (arg, NULL, 0, &num, NULL) != LONGINT_OK) {
> > +    fprintf (stderr, _("%s: '%s' is not a numeric
value.\n"),
> > +             guestfs_int_program_name, arg);
> > +    show_error (EXIT_FAILURE);
> > +  }
> > +
> > +  if (num <= INT_MIN || num >= INT_MAX) {
> > +    fprintf (stderr, _("%s: %s: integer out of range\n"),
> > +             guestfs_int_program_name, arg);
> > +    show_error (EXIT_FAILURE);
> 
> These bounds are not tight enough.  On 32 bit they should check the
> range of a 31 bit signed int, and on 64 bit, a 63 bit signed int.
Right, so -(2LL<<30) to (2LL<<30)-1 -- updating the patch
accordingly.

Thanks,
-- 
Pino Toscano

On Mon, Jul 18, 2016 at 10:47:12AM +0200, Pino Toscano
wrote:
> I still don't understand why we need to handle values bigger than int
> (as in C int, i.e. signed 32 bits) at all -- neither it is actually
> needed, nor it would be coherent in 32bit vs 64bit builds.
If that's what you decide then that is fine too.  My test patch
checked that 63 bit integers could be returned (on 64 bit arch) but we
can drop that test.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Monday, 18 July 2016 09:49:37 CEST Richard W.M. Jones
wrote:
> On Mon, Jul 18, 2016 at 10:47:12AM +0200, Pino Toscano wrote:
> > I still don't understand why we need to handle values bigger than
int
> > (as in C int, i.e. signed 32 bits) at all -- neither it is actually
> > needed, nor it would be coherent in 32bit vs 64bit builds.
> 
> If that's what you decide then that is fine too.  My test patch
> checked that 63 bit integers could be returned (on 64 bit arch) but we
> can drop that test.
Now that I think more about it, allowing more than 31bit integers as
'int' would cause problems later on; let's take for example the
handling of memsize in few OCaml tools:

  [ "-m"; "--memsize" ], Getopt.Int ("mb",
set_memsize), ...

later on, in the C implementation of Guestfs.set_memsize there is

  int memsize = Int_val (memsizev);

this would truncated when extracted, so not much useful at this point
already; even when changing the above, what follows is:

  r = guestfs_set_memsize (g, memsize);

considering the C API takes a 32bit signed integer, it would be
truncated anyway.  In the end, allowing big integers for
Getopt.Int/Set_int is not really doable/worth it.

-- 
Pino Toscano