Eric: Yifan Gu has posted a few patches for mingw support. My comments below. https://github.com/gyf304/nbdkit/commit/a37c4ca6546dfc4e96e305af97b62e5a9d6174ca * I think the SHARED_LDFLAGS idea is good. I pushed a slightly different take on the idea here: https://github.com/libguestfs/nbdkit/commit/1d634009ab8e43592065ec469df6312400525cc8 It's slightly different from what Yifan posted above, because I replaced -module -avoid-version -shared with $(SHARED_LDFLAGS), adding -no-undefined additionally on mingw. * Change to .gitignore is obviously fine, but I kept the file sorted: https://github.com/libguestfs/nbdkit/commit/002da4f4ca279a798fea1bf8749558d643d7c186 * Defining -D_spawnv=spawnv seems OK, but should be a separate patch. I didn't push anything. * Unclear why AM_CONDITIONAL(MINGW) is needed. It doesn't seem to be used anywhere unless I'm missing something. However if conditional compilation for mingw is needed for something, then this would be fine. * Unclear why it's necessary to rename nbdkit-common.h -> nbdkit-compat.h ? While this isn't API, it seems we should not rename it unless there's a good reason. * Functions are indirected through struct nbdkit_functions _nbdkit_functions which I guess is necessary because of PE linking (we've had similar problems with PE linking restrictions in other programs). However I wasn't very clear how this actually works, and we cannot break existing Linux binaries, so this needs more thought and must be conditional on Windows only. https://github.com/gyf304/nbdkit/commit/4f238c3e52a70b855dbb5f3eb3084d357b7c8071 * This one should be folded into the eventual nbdkit_functions patch. https://github.com/gyf304/nbdkit/commit/20d888fbb8ccf33f0975a070df0d8e3d929b71f8 * Is there no way to get the socket peer on Windows? 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 Sat, Mar 21, 2020 at 12:06:15PM +0000, Richard W.M. Jones wrote:> https://github.com/gyf304/nbdkit/commit/20d888fbb8ccf33f0975a070df0d8e3d929b71f8 > > * Is there no way to get the socket peer on Windows?And assuming there's no way to get the socket peer, then the function shouldn't be NULL (which will cause a segfault), but instead you should make the function return an error. eg: int nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen) { +#ifndef WINDOWS [... existing code unchanged ...] +#else + nbdkit_error ("returning peer name is not supported on Windows"); + return -1; +#endif } If the problem is that msys doesn't have struct sockaddr or socklen_t, then I guess conditionally #defining them? Eric knows about this better than I do. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Answering a few questions: * Unclear why it's necessary to rename nbdkit-common.h -> nbdkit-compat.h - It's not a rename, but a new file. nbdkit-compat.h is included for PE DLL compatibility, and is included in nbdkit-common.h * Functions are indirected through struct nbdkit_functions _nbdkit_functions which I guess is necessary because of PE linking (we've had similar problems with PE linking restrictions in other programs). However I wasn't very clear how this actually works, and we cannot break existing Linux binaries, so this needs more thought and must be conditional on Windows only. - There are macro checks for an windows environment. If it's windows, then indirection is used. Otherwise, the original extern definitions are used. This shouldn't cause an ABI change on Linux. * Is there no way to get the socket peer on Windows? - It's not a lack of ways that prevents me from implementing this. The nbdkit port is to MSYS2 (which is a superset of mingw-64), and this is needed because nbdkit uses an extensive set of POSIX features that are not present in mingw-64, e.g. fork). This port requires MSYS2 to run. However, the user might choose to implement the plugins / filters using mingw-64, or just MSVC runtime, and I don't want to bind users to MSYS2. Now this causes a potential ABI incompatibility: MSYS2 socket definitions are provided by sys/socket.h, but mingw-64 / MSVC socket definitions are usually provided by winsock.h or winsock2.h. To prevent possible issues, I removed nbdkit_peer_name. This function is not defined in the include headers if Windows is detected, so it will cause a compile time error if someone attempts to use it. On Sat, Mar 21, 2020 at 1:20 PM Richard W.M. Jones <rjones@redhat.com> wrote:> On Sat, Mar 21, 2020 at 12:06:15PM +0000, Richard W.M. Jones wrote: > > > https://github.com/gyf304/nbdkit/commit/20d888fbb8ccf33f0975a070df0d8e3d929b71f8 > > > > * Is there no way to get the socket peer on Windows? > > And assuming there's no way to get the socket peer, then the function > shouldn't be NULL (which will cause a segfault), but instead you > should make the function return an error. eg: > > int > nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen) > { > +#ifndef WINDOWS > [... existing code unchanged ...] > +#else > + nbdkit_error ("returning peer name is not supported on Windows"); > + return -1; > +#endif > } > > If the problem is that msys doesn't have struct sockaddr or socklen_t, > then I guess conditionally #defining them? Eric knows about this > better than I do. > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat > http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-builder quickly builds VMs from scratch > http://libguestfs.org/virt-builder.1.html > >
On 3/21/20 7:06 AM, Richard W.M. Jones wrote:> Eric: > > Yifan Gu has posted a few patches for mingw support. My comments > below. > > https://github.com/gyf304/nbdkit/commit/a37c4ca6546dfc4e96e305af97b62e5a9d6174ca > > * I think the SHARED_LDFLAGS idea is good. I pushed a slightly > different take on the idea here: > https://github.com/libguestfs/nbdkit/commit/1d634009ab8e43592065ec469df6312400525cc8 > It's slightly different from what Yifan posted above, because I > replaced -module -avoid-version -shared with $(SHARED_LDFLAGS), > adding -no-undefined additionally on mingw.Why are we trying to avoid -no-undefined on other platforms? I do think it's annoying the way libtool picked their defaults: the lowest-common denominator in shared libraries is avoiding undefined symbols, while many platforms such as Linux permit it as extension. But instead of making libtool default to the lowest common denominator, instead, you HAVE to specify -no-undefined on platforms where undefined does not work, and on other platforms, the absence of -no-undefined lets you get away with extensions, while the explicit use of -no-undefined tells your toolchain to verify that indeed you are following the stricter compatibility setup of no undefined symbols. But if a strict setup is necessary for building on mingw, being able to validate on Linux that we are obeying a strict setup seems like it would be a good thing (then Linux builds can tell us if we accidentally introduce an undefined symbol dependency), rather than waiting until someone reports the problem on less-tested mingw. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Mar 24, 2020 at 01:43:52PM -0500, Eric Blake wrote:> On 3/21/20 7:06 AM, Richard W.M. Jones wrote: > >Eric: > > > >Yifan Gu has posted a few patches for mingw support. My comments > >below. > > > >https://github.com/gyf304/nbdkit/commit/a37c4ca6546dfc4e96e305af97b62e5a9d6174ca > > > >* I think the SHARED_LDFLAGS idea is good. I pushed a slightly > > different take on the idea here: > > https://github.com/libguestfs/nbdkit/commit/1d634009ab8e43592065ec469df6312400525cc8 > > It's slightly different from what Yifan posted above, because I > > replaced -module -avoid-version -shared with $(SHARED_LDFLAGS), > > adding -no-undefined additionally on mingw. > > Why are we trying to avoid -no-undefined on other platforms?Isn't it because we rely on it, since our plugins need symbols that are undefined at link time such as nbdkit_*? Rich. -- 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