Re: [Libguestfs] Note regarding bin2s.pl
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. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
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? Rich. -- 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/
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. - 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); But either way, I'm fine with both approaches. Thanks! Helge
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-sizeofc... 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/
Control: close -1 5.1.14-2 * Richard W.M. Jones:
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.
We'll know about warnings on non-x86 Linux architectures in a couple of hours: I have just uploaded 5.1.14-2 which contains this commit. I'm therefore closing this bug report. Cheers, -Hilko
participants (3)
-
Helge Deller -
Hilko Bengen -
Richard W.M. Jones