Libguestfs - Feb 2015 - Re: Patchable build problems on OS X 10.10

On Friday 06 February 2015 10:03:37 Richard W.M. Jones
wrote:
> On Thu, Feb 05, 2015 at 10:53:06PM +0000, Margaret Lewicka wrote:
> > Hello,
> > 
> > I'm attempting to create a Homebrew formula to get libguestfs to
> > compile on Mac OS X. I've managed to achieve success with several
> > monkey patches, but since Homebrew's policy is to contact
maintainers
> > about proper fixes in upstream, I would like to ask if there are any
> > plans to fix these issues. I'm afraid I don't know C well
enough to
> > propose decent solutions myself.
> 
> Thanks for looking at this.  I am interested in getting libguestfs to
> work on Mac OS X again.
Thanks for your patches! In the past months I've done a number of
commits to help making libguestfs (without guestfsd & appliance) build
and work on FreeBSD.
> I have included the patch below for review.  As a general comment,
> it's easier for us if the patch is split into separate reviewable
> commits, and then sent to the mailing list using 'git send-email'.
> 
> > diff --git fuse/guestunmount.c fuse/guestunmount.c
> > index c36c336..1ab7ff5 100644
> > --- fuse/guestunmount.c
> > +++ fuse/guestunmount.c
> > @@ -257,7 +257,7 @@ do_fusermount (const char *mountpoint, char
**error_rtn)
> >      /* We have to parse error messages from fusermount, so ... */
> >      setenv ("LC_ALL", "C", 1);
> >  
> > -    execlp ("fusermount", "fusermount",
"-u", mountpoint, NULL);
> > +    execlp ("/sbin/umount", "umount", mountpoint,
NULL);
> 
> fusermount is needed on Linux, so this is an example of a patch
> which could be written instead as:
> 
>   #if !(defined __APPLE__ && defined __MACH__)
>       execlp ("fusermount", "fusermount",
"-u", mountpoint, NULL);
>   #else
>       execlp ("/sbin/umount", "umount", mountpoint,
NULL);
>   #endif
Or rather use fusermount only on Linux and umount (with no path)
elsewhere.
> > diff --git src/launch-libvirt.c src/launch-libvirt.c
> > index aaa8501..c0adc80 100644
> > --- src/launch-libvirt.c
> > +++ src/launch-libvirt.c
> > @@ -57,6 +57,18 @@
> >  #include "guestfs-internal-actions.h"
> >  #include "guestfs_protocol.h"
> >  
> > +/* Fixes for Mac OS X */
> > +#if defined __APPLE__ && defined __MACH__
> > +#include <sys/un.h>
> > +#endif
> > +#ifndef SOCK_CLOEXEC
> > +# define SOCK_CLOEXEC O_CLOEXEC
> > +#endif
> > +#ifndef SOCK_NONBLOCK
> > +# define SOCK_NONBLOCK O_NONBLOCK
> > +#endif
> > +/* End of fixes for Mac OS X */
> > +
> >  /* Check minimum required version of libvirt.  The libvirt backend
> >   * is new and not the default, so we can get away with forcing
> >   * people who want to try it to have a reasonably new version of
This IMHO is clearly wrong: the O_* constants are for open() & friends,
not for socket & socket4.
Theoretically, we could switch the socket() usages in launch-libvirt.c
to socket4(), which can be replaced by gnulib if missing (we already
use the "accept4" gnulib module). On the other hand, it seems that
such gnulib emulation does not provide SOCK_NONBLOCK, so either
a) fix that in gnulib
b) use the "nonblocking" gnulib module, using set_nonblocking_flag()
   instead of SOCK_NONBLOCK

Also, sys/un.h is POSIX [1], so it can be included unconditionally.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_un.h.html

Thanks,
-- 
Pino Toscano

On Mon, Feb 09, 2015 at 10:56:54AM +0100, Pino Toscano
wrote:
> On Friday 06 February 2015 10:03:37 Richard W.M. Jones wrote:
> > On Thu, Feb 05, 2015 at 10:53:06PM +0000, Margaret Lewicka wrote:
> > > +/* Fixes for Mac OS X */
> > > +#if defined __APPLE__ && defined __MACH__
> > > +#include <sys/un.h>
> > > +#endif
> > > +#ifndef SOCK_CLOEXEC
> > > +# define SOCK_CLOEXEC O_CLOEXEC
> > > +#endif
> > > +#ifndef SOCK_NONBLOCK
> > > +# define SOCK_NONBLOCK O_NONBLOCK
> > > +#endif
> > > +/* End of fixes for Mac OS X */
> > > +
> > >  /* Check minimum required version of libvirt.  The libvirt
backend
> > >   * is new and not the default, so we can get away with forcing
> > >   * people who want to try it to have a reasonably new version of
> 
> This IMHO is clearly wrong: the O_* constants are for open() & friends,
> not for socket & socket4.
I checked this out before committing it, and I accepted it because on
Linux/glibc, SOCK_CLOEXEC == O_CLOEXEC and SOCK_NONBLOCK == O_NONBLOCK
(see the definitions in bits/socket.h and bits/socket_type.h on a
Linux system).

Of course this is not a law of nature and there could be systems where
this is not true.

The macros as defined only affect systems that don't define SOCK_* at
all.
> Theoretically, we could switch the socket() usages in launch-libvirt.c
> to socket4(), which can be replaced by gnulib if missing (we already
> use the "accept4" gnulib module). On the other hand, it seems
that
> such gnulib emulation does not provide SOCK_NONBLOCK, so either
> a) fix that in gnulib
> b) use the "nonblocking" gnulib module, using
set_nonblocking_flag()
>    instead of SOCK_NONBLOCK
(b) would not be atomic.  I'm not certain if there's a case where that
matters for O_NONBLOCK.  (Of course it definitely matters for
O_CLOEXEC and we've have bugs caused by that in the past)
> Also, sys/un.h is POSIX [1], so it can be included unconditionally.
Agreed.

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 February 2015 at 09:56, Pino Toscano <ptoscano@redhat.com>
wrote:
> On Friday 06 February 2015 10:03:37 Richard W.M. Jones wrote:
[...]
>>   #if !(defined __APPLE__ && defined __MACH__)
>>       execlp ("fusermount", "fusermount",
"-u", mountpoint, NULL);
>>   #else
>>       execlp ("/sbin/umount", "umount", mountpoint,
NULL);
>>   #endif
>
> Or rather use fusermount only on Linux and umount (with no path)
> elsewhere.
I'm about to mail some patches, your reply arrived just in time. :)
Is #ifdef __linux__ sufficient in this case, then? I don't want to
make too general assumptions.

-- 
M.

On Mon, Feb 09, 2015 at 10:27:21AM +0000, Richard W.M. Jones
wrote:
> On Mon, Feb 09, 2015 at 10:56:54AM +0100, Pino Toscano wrote:
> > On Friday 06 February 2015 10:03:37 Richard W.M. Jones wrote:
> > > On Thu, Feb 05, 2015 at 10:53:06PM +0000, Margaret Lewicka wrote:
> > > > +/* Fixes for Mac OS X */
> > > > +#if defined __APPLE__ && defined __MACH__
> > > > +#include <sys/un.h>
> > > > +#endif
> > > > +#ifndef SOCK_CLOEXEC
> > > > +# define SOCK_CLOEXEC O_CLOEXEC
> > > > +#endif
> > > > +#ifndef SOCK_NONBLOCK
> > > > +# define SOCK_NONBLOCK O_NONBLOCK
> > > > +#endif
> > > > +/* End of fixes for Mac OS X */
> > > > +
> > > >  /* Check minimum required version of libvirt.  The libvirt
backend
> > > >   * is new and not the default, so we can get away with
forcing
> > > >   * people who want to try it to have a reasonably new
version of
> > 
> > This IMHO is clearly wrong: the O_* constants are for open() &
friends,
> > not for socket & socket4.
> 
> I checked this out before committing it, and I accepted it because on
> Linux/glibc, SOCK_CLOEXEC == O_CLOEXEC and SOCK_NONBLOCK == O_NONBLOCK
> (see the definitions in bits/socket.h and bits/socket_type.h on a
> Linux system).
> 
> Of course this is not a law of nature and there could be systems where
> this is not true.
> 
> The macros as defined only affect systems that don't define SOCK_* at
> all.
> 
> > Theoretically, we could switch the socket() usages in launch-libvirt.c
> > to socket4(), which can be replaced by gnulib if missing (we already
> > use the "accept4" gnulib module). On the other hand, it
seems that
> > such gnulib emulation does not provide SOCK_NONBLOCK, so either
> > a) fix that in gnulib
> > b) use the "nonblocking" gnulib module, using
set_nonblocking_flag()
> >    instead of SOCK_NONBLOCK
> 
> (b) would not be atomic.  I'm not certain if there's a case where
that
> matters for O_NONBLOCK.  (Of course it definitely matters for
> O_CLOEXEC and we've have bugs caused by that in the past)
A while ago there was someone trying to port libguestfs to Windows who
was also doing crazy hacks related to sockets code. IMHO both of these
are a indicator to use gnulib for sockets portability.  The atomicity
of setting SOCK_NONBLOCK doesn't matter because the socket() call does
not do anything except allocate the resource - you need a bind/connect
call before any action takes place, so you can safely set non-blocking
mode before those in a race-free manner.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

On Monday 09 February 2015 10:27:21 Richard W.M. Jones
wrote:
> On Mon, Feb 09, 2015 at 10:56:54AM +0100, Pino Toscano wrote:
> > On Friday 06 February 2015 10:03:37 Richard W.M. Jones wrote:
> > > On Thu, Feb 05, 2015 at 10:53:06PM +0000, Margaret Lewicka wrote:
> > > > +/* Fixes for Mac OS X */
> > > > +#if defined __APPLE__ && defined __MACH__
> > > > +#include <sys/un.h>
> > > > +#endif
> > > > +#ifndef SOCK_CLOEXEC
> > > > +# define SOCK_CLOEXEC O_CLOEXEC
> > > > +#endif
> > > > +#ifndef SOCK_NONBLOCK
> > > > +# define SOCK_NONBLOCK O_NONBLOCK
> > > > +#endif
> > > > +/* End of fixes for Mac OS X */
> > > > +
> > > >  /* Check minimum required version of libvirt.  The libvirt
backend
> > > >   * is new and not the default, so we can get away with
forcing
> > > >   * people who want to try it to have a reasonably new
version of
> > 
> > This IMHO is clearly wrong: the O_* constants are for open() &
friends,
> > not for socket & socket4.
> 
> I checked this out before committing it, and I accepted it because on
> Linux/glibc, SOCK_CLOEXEC == O_CLOEXEC and SOCK_NONBLOCK == O_NONBLOCK
> (see the definitions in bits/socket.h and bits/socket_type.h on a
> Linux system).
> 
> Of course this is not a law of nature and there could be systems where
> this is not true.
> 
> The macros as defined only affect systems that don't define SOCK_* at
> all.
Yes, I know SOCK_CLOEXEC == O_CLOEXEC on Linux, but on libc's without
such flags passing them might just cause accept/accept4 to fail
at runtime with EPROTOTYPE or EINVAL.
> > Theoretically, we could switch the socket() usages in launch-libvirt.c
> > to socket4(), which can be replaced by gnulib if missing (we already
> > use the "accept4" gnulib module). On the other hand, it
seems that
> > such gnulib emulation does not provide SOCK_NONBLOCK, so either
> > a) fix that in gnulib
> > b) use the "nonblocking" gnulib module, using
set_nonblocking_flag()
> >    instead of SOCK_NONBLOCK
> 
> (b) would not be atomic.
I'm not sure how it matters, since socket() just creates the socket
without doing anything further.

-- 
Pino Toscano

On Mon, Feb 09, 2015 at 10:29:20AM +0000, Margaret Lewicka
wrote:
> On 9 February 2015 at 09:56, Pino Toscano <ptoscano@redhat.com>
wrote:
> > On Friday 06 February 2015 10:03:37 Richard W.M. Jones wrote:
> [...]
> >>   #if !(defined __APPLE__ && defined __MACH__)
> >>       execlp ("fusermount", "fusermount",
"-u", mountpoint, NULL);
> >>   #else
> >>       execlp ("/sbin/umount", "umount",
mountpoint, NULL);
> >>   #endif
> >
> > Or rather use fusermount only on Linux and umount (with no path)
> > elsewhere.
> 
> I'm about to mail some patches, your reply arrived just in time. :)
> Is #ifdef __linux__ sufficient in this case, then? I don't want to
> make too general assumptions.
For this specific patch, yes.

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