[Pkg-libvirt-maintainers] Bug#809185: [Libguestfs] Note regarding bin2s.pl

[Richard W.M. Jones]

On Tue, Jan 12, 2016 at 06:21:14PM +0100, Helge Deller wrote:
> On 12.01.2016 12:10, Richard W.M. Jones wrote:
> > On Tue, Jan 12, 2016 at 10:05:00AM +0000, Richard W.M. Jones wrote:
> >> On Tue, Jan 12, 2016 at 07:57:03AM +0100, Hilko Bengen wrote:
> >>> Helge,
> >>>
> >>> I have applied all the architecture-specific bits but not the bin2s
> >>> script yet. TBH, so far I don't see what is wrong about export and use
> >>> of the "_binary_init_size" constant.
> >>
> >> [https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=809185]
> >>
> >> I see it as a reasonable simplification - it allows us to get rid of
> >> that conditional code for HP-UX in bin2s.pl.
> >>
> >> However looking at the patch, I don't like the casts in:
> >>
> >> -  size_t n = (size_t) &_binary_init_size;
> >> +  size_t n = ((size_t) &_binary_init_end) - ((size_t) &_binary_init_start);
> >>
> >> Since those are pointers, it seems better to simply subtract them.
> >> (Though it would be better if we'd declared the type of
> >> _binary_init_start/_end as uint8_t instead of char.)
> >>
> >> If we must cast them then the correct integer to use is 'intptr_t', an
> >> int type that's guaranteed by C99 to be long enough to store a
> >> pointer.
> > 
> > How about the attached patch?
> 
> In general I'd say it looks OK.
> Just a few comments:
> 
> -extern char _binary_init_start, _binary_init_end, _binary_init_size;
> +extern uint8_t _binary_init_start, _binary_init_end;
> 
> Does the char to uint8_t change really makes such a big difference?
> We will just use the address of the variable anyway.  

It seems from this answer:

https://stackoverflow.com/questions/2215445/are-there-machines-where-sizeofchar-1-or-at-least-char-bit-8

that sizeof (char) is always 1 so it would never make a difference.
However I suppose it's better to be clearer about what we intend.

> -  size_t n = (size_t) &_binary_init_size;
> +  size_t n = &_binary_init_end - &_binary_init_start;
> 
> It's OK, but maybe some compilers/platforms might complain with a warning.
> It might be better to keep a cast to (size_t), e.g.:
> + size_t n = (size_t) (&_binary_init_end - &_binary_init_start);

I tested the patch today on 32- and 64-bit Linux x86 systems, but
nothing beyond that.

Actually I think if it warns, I want to know about that -- eg. in some
bizarre case where size_t isn't sufficient to store the result.

Patch applied anyway, thanks.

Rich.

> But either way, I'm fine with both approaches.
> 
> Thanks!
> Helge

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/