Libguestfs - Sep 2018 - Re: [PATCH nbdkit 1/3] common: isaligned: Use uint64_t instead of unsigned int.

On Mon, Sep 17, 2018 at 6:03 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
> This should have no effect.  However it's probably better to pass the
> full type explicitly rather than using an implicit truncation.
>
Why is it better?

We care only about the least significant bits; current code is clear about
that.

This is also specified behavior, although the only reference I can find now
is this:
https://docs.microsoft.com/en-us/cpp/c-language/conversions-from-unsigned-integral-types?view=vs-2017

Nir

> ---
>  common/include/isaligned.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/common/include/isaligned.h b/common/include/isaligned.h
> index e693820..9ed635e 100644
> --- a/common/include/isaligned.h
> +++ b/common/include/isaligned.h
> @@ -36,13 +36,14 @@
>
>  #include <assert.h>
>  #include <stdbool.h>
> +#include <stdint.h>
>
>  #include "ispowerof2.h"
>
>  /* Return true if size is a multiple of align. align must be power of 2.
>   */
>  static inline bool
> -is_aligned (unsigned int size, unsigned int align)
> +is_aligned (uint64_t size, unsigned int align)
>  {
>    assert (is_power_of_2 (align));
>    return !(size & (align - 1));
> --
> 2.19.0.rc0
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
>

On Mon, Sep 17, 2018 at 10:00:30PM +0300, Nir Soffer
wrote:
> On Mon, Sep 17, 2018 at 6:03 PM Richard W.M. Jones
<rjones@redhat.com>
> wrote:
> 
> > This should have no effect.  However it's probably better to pass
the
> > full type explicitly rather than using an implicit truncation.
> >
> 
> Why is it better?
It's not necessarily, hence patches are posted for review :-)

My other idea was to turn it into a macro, something like this:

  #define is_aligned(size, align) (!((size) & ((align) - 1)))

This, I believe, is independent of the int type used by the caller so
achieves the same end.  However it unfortunately drops the assertion
and I couldn't think of a good way to add the assertion to this
expression without using GCC extensions.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Mon, Sep 17, 2018 at 08:55:06PM +0100, Richard W.M. Jones
wrote:
> On Mon, Sep 17, 2018 at 10:00:30PM +0300, Nir Soffer wrote:
> > On Mon, Sep 17, 2018 at 6:03 PM Richard W.M. Jones
<rjones@redhat.com>
> > wrote:
> > 
> > > This should have no effect.  However it's probably better to
pass the
> > > full type explicitly rather than using an implicit truncation.
> > >
> > 
> > Why is it better?
OK I'll give one (somewhat contrived) example where it might be better
not to rely on implicit truncation of parameters:

(1) User is compiling on a 32 bit architecture.

(2) They have turned off or are ignoring compiler warnings.

(3) The code has forgotten to #include "isaligned.h" (either our code
omits this, which is unlikely, or the user is developing a new plugin
using the same framework).

In this case the uint64_t would be passed as a 64 bit value (eg.  in
two registers, or as a 64 bit stack entry), and thus the receiving
function would get the wrong parameters.

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 9/17/18 2:55 PM, Richard W.M. Jones wrote:
> On Mon, Sep 17, 2018 at 10:00:30PM +0300, Nir Soffer wrote:
>> On Mon, Sep 17, 2018 at 6:03 PM Richard W.M. Jones
<rjones@redhat.com>
>> wrote:
>>
>>> This should have no effect.  However it's probably better to
pass the
>>> full type explicitly rather than using an implicit truncation.
>>>
>>
>> Why is it better?
> 
> It's not necessarily, hence patches are posted for review :-)
> 
> My other idea was to turn it into a macro, something like this:
> 
>    #define is_aligned(size, align) (!((size) & ((align) - 1)))
> 
> This, I believe, is independent of the int type used by the caller so
> achieves the same end.  However it unfortunately drops the assertion
> and I couldn't think of a good way to add the assertion to this
> expression without using GCC extensions.
Then again, we already rely on gcc/clang features (such as automatic 
variable cleanup on scope exit), so it's highly probable that any such 
extension we use is already supported by any compiler that can already 
grok nbdkit sources.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org