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 Sat, Mar 21, 2020 at 09:36:15PM -0400, Frank Gu wrote:> 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.hOh I see. The github UI is super-confusing and made it look as if the file had been renamed. Is it possible at some point that you could rebase your work on top of the current head, and split it into individually reviewable patches? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
The rebased version is at
https://github.com/gyf304/nbdkit/commits/msys2-port
It is currently buildable with:
autoreconf -i && ./configure host=x86_64-pc-msys && make -j
A few changes are added:
* nbdkit_peer_name is added back but will always error.
* __declspec(dllexport) is added to the headers for PE compatibility.
* -D_spawnv=spawnv is now removed.
I think I didn't run ./configure properly last time.
Here is the list of features that are tested to compile.
Optional server features:
bash-completion ........................ yes
manual pages ........................... yes
SELinux ................................ no
TLS .................................... no
Optional plugins:
curl ................................... no
example4 ............................... yes
ext2 ................................... no
floppy ................................. no
guestfs ................................ no
gzip ................................... no
iso .................................... no
libvirt ................................ no
nbd .................................... no
ssh .................................... no
tar .................................... yes
vddk ................................... yes
Languages:
lua .................................... no
ocaml .................................. no
perl ................................... yes
python ................................. yes
ruby ................................... no
rust ................................... no
tcl .................................... no
Optional filters:
ext2 ................................... no
xz ..................................... no
On Sun, Mar 22, 2020 at 10:28 AM Richard W.M. Jones <rjones@redhat.com>
wrote:
> On Sat, Mar 21, 2020 at 09:36:15PM -0400, Frank Gu wrote:
> > 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
>
> Oh I see. The github UI is super-confusing and made it look as if the
> file had been renamed.
>
> Is it possible at some point that you could rebase your work on top of
> the current head, and split it into individually reviewable patches?
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
> http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> Fedora Windows cross-compiler. Compile Windows programs, test, and
> build Windows installers. Over 100 libraries supported.
> http://fedoraproject.org/wiki/MinGW
>
>