Libguestfs - Jan 2019 - Re: [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT_NEEDED for C90 compilers.

On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
> When an ANSI/C90 plugin compiled with ‘-pedantic’ uses
> NBDKIT_HANDLE_NOT_NEEDED it gets the error:
> 
>   ISO C forbids conversion of function pointer to object pointer type
While POSIX requires it to work. But such is life.
> 
> This is because the existing macro worked by returning a function
> pointer but in C90 function pointers cannot be cast to data pointers
> since on some ancient architectures code and data pointers were
> incompatible.
> 
> We only need a convenient global data pointer here, and the address of
> ‘errno’ should be fine.
errno is often implemented as a dereference of a function call in order
to get thread-safe semantics; for example, your patch produces
'&(*__errno_location())' on glibc.  That should be okay (especially
since it silenced the warning).

Another option, instead of referencing an actual variable, could be
writing ((void*)(intptr_t)1).

Either way, this patch makes sense.
> @@ -78,7 +79,7 @@ extern char *nbdkit_realpath (const char *path);
>  /* A static non-NULL pointer which can be used when you don't need a
>   * per-connection handle.
>   */
> -#define NBDKIT_HANDLE_NOT_NEEDED ((void *) &nbdkit_error)
> +#define NBDKIT_HANDLE_NOT_NEEDED (&errno)
>  
>  #ifdef __cplusplus
>  }
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Fri, Jan 18, 2019 at 08:36:07AM -0600, Eric Blake
wrote:
> On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
> > When an ANSI/C90 plugin compiled with ‘-pedantic’ uses
> > NBDKIT_HANDLE_NOT_NEEDED it gets the error:
> > 
> >   ISO C forbids conversion of function pointer to object pointer type
> 
> While POSIX requires it to work. But such is life.
> 
> > 
> > This is because the existing macro worked by returning a function
> > pointer but in C90 function pointers cannot be cast to data pointers
> > since on some ancient architectures code and data pointers were
> > incompatible.
> > 
> > We only need a convenient global data pointer here, and the address of
> > ‘errno’ should be fine.
> 
> errno is often implemented as a dereference of a function call in order
> to get thread-safe semantics; for example, your patch produces
> '&(*__errno_location())' on glibc.  That should be okay
(especially
> since it silenced the warning).
Yeah, I was aware of this and looking for some other global variable
but couldn't think of one at the time.

Hmm, how about optind?

Rich.
> Another option, instead of referencing an actual variable, could be
> writing ((void*)(intptr_t)1).
> 
> Either way, this patch makes sense.
> 
> > @@ -78,7 +79,7 @@ extern char *nbdkit_realpath (const char *path);
> >  /* A static non-NULL pointer which can be used when you don't
need a
> >   * per-connection handle.
> >   */
> > -#define NBDKIT_HANDLE_NOT_NEEDED ((void *) &nbdkit_error)
> > +#define NBDKIT_HANDLE_NOT_NEEDED (&errno)
> >  
> >  #ifdef __cplusplus
> >  }
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 1/18/19 8:47 AM, Richard W.M. Jones wrote:
> On Fri, Jan 18, 2019 at 08:36:07AM -0600, Eric Blake wrote:
>> On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
>>> When an ANSI/C90 plugin compiled with ‘-pedantic’ uses
>>> NBDKIT_HANDLE_NOT_NEEDED it gets the error:
>>>
>>>   ISO C forbids conversion of function pointer to object pointer
type
>>
>> While POSIX requires it to work. But such is life.
>>
>>>
>>> This is because the existing macro worked by returning a function
>>> pointer but in C90 function pointers cannot be cast to data
pointers
>>> since on some ancient architectures code and data pointers were
>>> incompatible.
>>>
>>> We only need a convenient global data pointer here, and the address
of
>>> ‘errno’ should be fine.
>>
>> errno is often implemented as a dereference of a function call in order
>> to get thread-safe semantics; for example, your patch produces
>> '&(*__errno_location())' on glibc.  That should be okay
(especially
>> since it silenced the warning).
> 
> Yeah, I was aware of this and looking for some other global variable
> but couldn't think of one at the time.
> 
> Hmm, how about optind?
<getopt.h> is not portable, and I'm not sure if <unistd.h>
universally
declares optind.  gnulib states that MSVC 14 lacks <unistd.h>.

Other POSIX standardized global variables:
environ (but no standardized header declares it)
daylight, getdate_err, timezone, tzname (<time.h>, but both marked XSI
so not necessarily on all platforms)
optarg, opterr, optopt (<unistd.h>, same boat as optind)
signgam (<math.h>, but marked XSI, and requires -lm on some platforms)
stdin, stdout, stderr (<stdio.h>)

Also, pulling in the address of one of these forces the variable to be
linked in. For errno or stdin, that's okay (you're probably linking it
in anyways), but for the others, it may (slightly) bloat the resulting
binary, by referencing an otherwise unused variable.
> 
> Rich.
> 
>> Another option, instead of referencing an actual variable, could be
>> writing ((void*)(intptr_t)1).
>>
>> Either way, this patch makes sense.
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org