Libguestfs - Aug 2020 - [nbdkit] Windows errno handling

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

The shorter story is:

 - Windows functions like send, recv, etc. do not set errno.

 - In fact it's not even possible for the concept of a shared global
   variable to exist with Windows DLLs, or at least not without a
   bunch of work:
  
(https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-6.0/aa270058(v=vs.60))

 - To get and set the thread-local error from the last function
   we must call WSAGetLastError and WSASetLastError.

 - Error codes are integers with superficial similarity to errno codes
   but different symbols and values (eg. WSAEINTR == 10004 is similar
   to EINTR).

 - I'm not sure if this is just a mingw thing or a Windows thing, but
   mingw defines an <errno.h> header which has its own values for
   errno like EINTR, and for some reason perror(3) cannot print many
   of those values.  Still looking into this one ...

nbdkit does approximately 5 different things with errno:

(1a) It reads them by reading the thread-local variable errno.

(1b) It reads and compares them, eg. errno == EINTR.

(2) It writes to them to set particular errno values, eg. errno = EIO.

(3) It saves them around functions such as nbdkit_error(), which
    is (1) + (2) but might be a distinct operation eg. using cleanups.

(4) It prints them with perror and %m.

(5) It handles plugins which claim errno_is_preserved.

My first thought was we could define a macro which does:

 #ifdef WIN32
 #define errno (translate_to_errno (WSAGetLastError ()))
 #endif

which reads the last error and translates WSA* to E* codes.  This
would solve (1) and is not very invasive for existing code.

We'd have to then need to wrap all assignments to errno
in a macro like:

 #ifndef WIN32
 #define set_errno(v) (errno = (v))
 #else
 #define set_errno(v) (WSASetLastError (translate_from_errno (v)))
 #endif

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
(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.

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 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
> 
> The shorter story is:
> 
>  - Windows functions like send, recv, etc. do not set errno.
> 
>  - In fact it's not even possible for the concept of a shared global
>    variable to exist with Windows DLLs, or at least not without a
>    bunch of work:
>   
(https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-6.0/aa270058(v=vs.60))
> 
>  - To get and set the thread-local error from the last function
>    we must call WSAGetLastError and WSASetLastError.
> 
>  - Error codes are integers with superficial similarity to errno codes
>    but different symbols and values (eg. WSAEINTR == 10004 is similar
>    to EINTR).
> 
>  - I'm not sure if this is just a mingw thing or a Windows thing, but
>    mingw defines an <errno.h> header which has its own values for
>    errno like EINTR, and for some reason perror(3) cannot print many
>    of those values.  Still looking into this one ...
> 
> nbdkit does approximately 5 different things with errno:
> 
> (1a) It reads them by reading the thread-local variable errno.
> 
> (1b) It reads and compares them, eg. errno == EINTR.
> 
> (2) It writes to them to set particular errno values, eg. errno = EIO.
> 
> (3) It saves them around functions such as nbdkit_error(), which
>     is (1) + (2) but might be a distinct operation eg. using cleanups.
> 
> (4) It prints them with perror and %m.
> 
> (5) It handles plugins which claim errno_is_preserved.
> 
> My first thought was we could define a macro which does:
> 
>  #ifdef WIN32
>  #define errno (translate_to_errno (WSAGetLastError ()))
>  #endif
> 
> which reads the last error and translates WSA* to E* codes.  This
> would solve (1) and is not very invasive for existing code.
> 
> We'd have to then need to wrap all assignments to errno
> in a macro like:
> 
>  #ifndef WIN32
>  #define set_errno(v) (errno = (v))
>  #else
>  #define set_errno(v) (WSASetLastError (translate_from_errno (v)))
>  #endif
> 
> 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
> (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.

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
-- 
|: 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 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).
> 
> 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 07:36:00PM +0100, Richard W.M. Jones
wrote:
> We'd have to then need to wrap all assignments to errno
> in a macro like:
> 
>  #ifndef WIN32
>  #define set_errno(v) (errno = (v))
>  #else
>  #define set_errno(v) (WSASetLastError (translate_from_errno (v)))
>  #endif
>
> This is very invasive for existing code.  There are ~60 places in the
> existing code which seem to assign to errno, ...
I think we _don't_ need to do this.  That's because we never actually
want to call WSASetLastError for a couple of reasons: (a) Just like
for Linux, no Windows API call would be affected by WSASetLastError.
(b) errno is a thread safe global variable on Windows which can also
be assigned
(https://stackoverflow.com/questions/6413052/msvc-errno-thread-safety).

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