On 8/17/20 1:46 PM, Daniel P. Berrangé wrote:> On Mon, Aug 17, 2020 at 07:36:00PM +0100, Richard W.M. Jones wrote: >> The Windows port of nbdkit >> (https://github.com/rwmjones/nbdkit/tree/2020-windows-mingw) now works >> to some extent. However errno handling doesn't work. The way that >> Winsock handles errors is incompatible with the way we expect to work >> errno in several ways. The long story is here: >> >> https://docs.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 >> https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 >>>> >> This is very invasive for existing code. There are ~60 places in the >> existing code which seem to assign to errno, but some of these either >> set errno = 0 or are preserving errno (item (3) above), and so we'd >> probably want to handle those a bit differently. >> >> Printing of Winsock codes through perror() or %m actually worksThat _is_ a surprise. But if it makes like easier, we'll take it.>> (surprisingly). However it does _not_ seem to work if we try to >> translate the codes to errno E* values. I need to look at exactly >> what's going on here. >> >> Number (5) is actually fairly easy to deal with because there's only >> one place where we handle the errno returned by plugins >> (server/plugins.c:get_error). I think we'd probably want >> errno_is_preserved to mean "WSAGetLastError" or "GetLastError" >> contains something of interest. >> >> Thoughts? >> >> Also I really need to look at how some other portable libraries like >> curl and gnutls are handling this. Maybe they've already come up with >> something. > > Take a look at what libvirt has done. We follow a simplified version > of what GNULIB does, by defining custom wrapper functions around > all winsock APIs we need that set errno, and then use a macro to > transparently replace calls to use our wrappers: > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virsocket.h > https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virsocket.c > > You should be able to just lift those two files straight into your > git repo as they have no deps on other libvirt infra.Except that libvirt is GPLv2+, and nbdkit is not, so we'd need to relicense (if all those lines are due to you, we may be safe; but if they derive too much from gnulib, we're stuck).> > QEMU follows a similar kind of approach too, but its impl is harder > to lift out. > > Note, we never use %m in libvirt so don't need to solve that particular > problem. QEMU never uses %m either except in the few files which are > 100% linux only and will never need to be portable. > > Regards, > Daniel >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Aug 17, 2020 at 01:56:10PM -0500, Eric Blake wrote:> On 8/17/20 1:46 PM, Daniel P. Berrangé wrote: > > On Mon, Aug 17, 2020 at 07:36:00PM +0100, Richard W.M. Jones wrote: > > > The Windows port of nbdkit > > > (https://github.com/rwmjones/nbdkit/tree/2020-windows-mingw) now works > > > to some extent. However errno handling doesn't work. The way that > > > Winsock handles errors is incompatible with the way we expect to work > > > errno in several ways. The long story is here: > > > > > > https://docs.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 > > > https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 > > > > > > > > > > This is very invasive for existing code. There are ~60 places in the > > > existing code which seem to assign to errno, but some of these either > > > set errno = 0 or are preserving errno (item (3) above), and so we'd > > > probably want to handle those a bit differently. > > > > > > Printing of Winsock codes through perror() or %m actually works > > That _is_ a surprise. But if it makes like easier, we'll take it. > > > > (surprisingly). However it does _not_ seem to work if we try to > > > translate the codes to errno E* values. I need to look at exactly > > > what's going on here. > > > > > > Number (5) is actually fairly easy to deal with because there's only > > > one place where we handle the errno returned by plugins > > > (server/plugins.c:get_error). I think we'd probably want > > > errno_is_preserved to mean "WSAGetLastError" or "GetLastError" > > > contains something of interest. > > > > > > Thoughts? > > > > > > Also I really need to look at how some other portable libraries like > > > curl and gnutls are handling this. Maybe they've already come up with > > > something. > > > > Take a look at what libvirt has done. We follow a simplified version > > of what GNULIB does, by defining custom wrapper functions around > > all winsock APIs we need that set errno, and then use a macro to > > transparently replace calls to use our wrappers: > > > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virsocket.h > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virsocket.c > > > > You should be able to just lift those two files straight into your > > git repo as they have no deps on other libvirt infra. > > Except that libvirt is GPLv2+, and nbdkit is not, so we'd need to relicense > (if all those lines are due to you, we may be safe; but if they derive too > much from gnulib, we're stuck).I wrote it all from scratch, but I was referencing the gnulib code, so can't claim its completely clean room, and the set_errno method was directly lifted from gnulib code. If nothing else you could do a similar kind of approach though. I felt it was preferrable to wrap the socket APIs in one place, instead of adding extra magic in every caller, so the cruft is isolated. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Aug 17, 2020 at 08:07:12PM +0100, Daniel P. Berrangé wrote:> On Mon, Aug 17, 2020 at 01:56:10PM -0500, Eric Blake wrote: > > On 8/17/20 1:46 PM, Daniel P. Berrangé wrote: > > > On Mon, Aug 17, 2020 at 07:36:00PM +0100, Richard W.M. Jones wrote: > > > > The Windows port of nbdkit > > > > (https://github.com/rwmjones/nbdkit/tree/2020-windows-mingw) now works > > > > to some extent. However errno handling doesn't work. The way that > > > > Winsock handles errors is incompatible with the way we expect to work > > > > errno in several ways. The long story is here: > > > > > > > > https://docs.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 > > > > https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 > > > > > > > > > > > > > > This is very invasive for existing code. There are ~60 places in the > > > > existing code which seem to assign to errno, but some of these either > > > > set errno = 0 or are preserving errno (item (3) above), and so we'd > > > > probably want to handle those a bit differently. > > > > > > > > Printing of Winsock codes through perror() or %m actually works > > > > That _is_ a surprise. But if it makes like easier, we'll take it. > > > > > > (surprisingly). However it does _not_ seem to work if we try to > > > > translate the codes to errno E* values. I need to look at exactly > > > > what's going on here. > > > > > > > > Number (5) is actually fairly easy to deal with because there's only > > > > one place where we handle the errno returned by plugins > > > > (server/plugins.c:get_error). I think we'd probably want > > > > errno_is_preserved to mean "WSAGetLastError" or "GetLastError" > > > > contains something of interest. > > > > > > > > Thoughts? > > > > > > > > Also I really need to look at how some other portable libraries like > > > > curl and gnutls are handling this. Maybe they've already come up with > > > > something. > > > > > > Take a look at what libvirt has done. We follow a simplified version > > > of what GNULIB does, by defining custom wrapper functions around > > > all winsock APIs we need that set errno, and then use a macro to > > > transparently replace calls to use our wrappers: > > > > > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virsocket.h > > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virsocket.c > > > > > > You should be able to just lift those two files straight into your > > > git repo as they have no deps on other libvirt infra. > > > > Except that libvirt is GPLv2+, and nbdkit is not, so we'd need to relicense > > (if all those lines are due to you, we may be safe; but if they derive too > > much from gnulib, we're stuck). > > I wrote it all from scratch, but I was referencing the gnulib code, > so can't claim its completely clean room, and the set_errno method > was directly lifted from gnulib code. > > If nothing else you could do a similar kind of approach though. I > felt it was preferrable to wrap the socket APIs in one place, instead > of adding extra magic in every caller, so the cruft is isolated.Thanks - I'll study the code to see if we can reimplement the technique anyhow. 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/