[PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

Experiments with damaged USB flash drives and ddrescue revealed that the 
current mapping of these Win32 errors to the fallback EACCES could be 
improved.

BTW: I wonder why EACCES was selected as the fallback. Source code 
control forensics suggest that this was decided in the last millennium. 
A related comment from CGF added August 2000 persists until today :-)
/* FIXME: what's so special about EACCESS? */

-- 
Regards,
Christian


[-- Attachment #2: 0001-Cygwin-Map-ERROR_NO_SUCH_DEVICE-and-ERROR_MEDIA_CHAN.patch --]
[-- Type: text/plain, Size: 1550 bytes --]

From 8aa19c7fd13dc3790dc271dede8954539bffcd4d Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Fri, 23 Feb 2024 19:01:09 +0100
Subject: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to
 ENODEV

If a removable (USB) device is disconnected after opening its raw
device, R/W attempts fail with ERROR_NO_SUCH_DEVICE(433).  If the
raw device of a partition is used, ERROR_MEDIA_CHANGED(1110) is
returned instead.  Both are mapped to ENODEV(19) because <errno.h>
does not offer a value which better matches ERROR_MEDIA_CHANGED.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/local_includes/errmap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/local_includes/errmap.h b/winsup/cygwin/local_includes/errmap.h
index 326b35b6c..a0b3ff400 100644
--- a/winsup/cygwin/local_includes/errmap.h
+++ b/winsup/cygwin/local_includes/errmap.h
@@ -438,7 +438,7 @@ static const int errmap[] =
   0,			/* 430 */
   0,			/* 431 */
   0,			/* 432 */
-  0,			/* 433 */
+  ENODEV,		/* ERROR_NO_SUCH_DEVICE */
   0,			/* 434 */
   0,			/* 435 */
   0,			/* 436 */
@@ -1115,7 +1115,7 @@ static const int errmap[] =
   0,			/* ERROR_DEVICE_NOT_PARTITIONED */
   0,			/* ERROR_UNABLE_TO_LOCK_MEDIA */
   0,			/* ERROR_UNABLE_TO_UNLOAD_MEDIA */
-  0,			/* ERROR_MEDIA_CHANGED */
+  ENODEV,		/* ERROR_MEDIA_CHANGED */
   EIO,			/* ERROR_BUS_RESET */
   ENOMEDIUM,		/* ERROR_NO_MEDIA_IN_DRIVE */
   0,			/* ERROR_NO_UNICODE_TRANSLATION */
-- 
2.43.0

Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[Corinna Vinschen]

On Feb 23 19:14, Christian Franke wrote:
> Experiments with damaged USB flash drives and ddrescue revealed that the
> current mapping of these Win32 errors to the fallback EACCES could be
> improved.
> 
> BTW: I wonder why EACCES was selected as the fallback. Source code control
> forensics suggest that this was decided in the last millennium. A related
> comment from CGF added August 2000 persists until today :-)
> /* FIXME: what's so special about EACCESS? */

This goes back until 1997 in pre-CVS times.  There's a ChangeLog entry

  Wed Oct 29 22:43:57 1997  Geoffrey Noer  <noer@cygnus.com>

        [...]
        * syscalls.cc (seterrno): on failure, set EACCES instead of EPERM
        which is better for the unknown error case

So the default was EPERM at first and has been changed to EACCES
because it "is better for the unknown error case".

I'm open to ideas for an improved error mapping.

> From 8aa19c7fd13dc3790dc271dede8954539bffcd4d Mon Sep 17 00:00:00 2001
> From: Christian Franke <christian.franke@t-online.de>
> Date: Fri, 23 Feb 2024 19:01:09 +0100
> Subject: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to
>  ENODEV
> 
> If a removable (USB) device is disconnected after opening its raw
> device, R/W attempts fail with ERROR_NO_SUCH_DEVICE(433).  If the
> raw device of a partition is used, ERROR_MEDIA_CHANGED(1110) is
> returned instead.  Both are mapped to ENODEV(19) because <errno.h>
> does not offer a value which better matches ERROR_MEDIA_CHANGED.
> 
> Signed-off-by: Christian Franke <christian.franke@t-online.de>
> ---
>  winsup/cygwin/local_includes/errmap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Pushed.

Thanks,
Corinna

Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[Christian Franke]

Corinna Vinschen wrote:
> On Feb 23 19:14, Christian Franke wrote:
>> Experiments with damaged USB flash drives and ddrescue revealed that the
>> current mapping of these Win32 errors to the fallback EACCES could be
>> improved.
>>
>> BTW: I wonder why EACCES was selected as the fallback. Source code control
>> forensics suggest that this was decided in the last millennium. A related
>> comment from CGF added August 2000 persists until today :-)
>> /* FIXME: what's so special about EACCESS? */
> This goes back until 1997 in pre-CVS times.  There's a ChangeLog entry
>
>    Wed Oct 29 22:43:57 1997  Geoffrey Noer  <noer@cygnus.com>
>
>          [...]
>          * syscalls.cc (seterrno): on failure, set EACCES instead of EPERM
>          which is better for the unknown error case
>
> So the default was EPERM at first and has been changed to EACCES
> because it "is better for the unknown error case".
>
> I'm open to ideas for an improved error mapping.

I have no better suggestion for a default errno. Adding a cygwin 
specific one (like ENMFILE, ENOSHARE and ECASECLASH added 2000-2001) is 
possibly not desired.

Some thoughts about minor improvements of the errmap.h file:
- Add error number to each /* ERROR_... */ comment, e.g. /* 2: 
ERROR_FILE_NOT_FOUND */.
- Update /* NUMBER */ comments using current MinGW-w64's winerror.h 
(~850 changes).
- Max errno is 143, so data type size could be reduced from int to 
uint8_t aka unsigned char. Could even add a compile time check by using 
C++11's braced initializers which do not allow narrowing conversions.
- Remove trailing entries which only map to 0.
- Append a static_assert which checks whether array size matches the 
last mapped error number.

I could provide separate patches if desired.

Thanks,
Christian

Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[Corinna Vinschen]

On Feb 25 10:12, Christian Franke wrote:
> Corinna Vinschen wrote:
> > So the default was EPERM at first and has been changed to EACCES
> > because it "is better for the unknown error case".
> > 
> > I'm open to ideas for an improved error mapping.
> 
> I have no better suggestion for a default errno. Adding a cygwin specific
> one (like ENMFILE, ENOSHARE and ECASECLASH added 2000-2001) is possibly not
> desired.

ENOSHARE and ECASECLASH are not used anymore, fortunately, and ENMFILE
is hopefully never returned to userspace.  It might be a good idea to
remove it from Cygwin's code as well.

> Some thoughts about minor improvements of the errmap.h file:
> - Add error number to each /* ERROR_... */ comment, e.g. /* 2:
> ERROR_FILE_NOT_FOUND */.

Ok.

> - Update /* NUMBER */ comments using current MinGW-w64's winerror.h (~850
> changes).

Why so many?  I used winerror.h to populate the list not too long ago,
so I wonder why it suddenly has so many more error codes?

> - Max errno is 143, so data type size could be reduced from int to uint8_t
> aka unsigned char. Could even add a compile time check by using C++11's
> braced initializers which do not allow narrowing conversions.

Yeah, we could do that.

> - Remove trailing entries which only map to 0.
> - Append a static_assert which checks whether array size matches the last
> mapped error number.

Yeah, not so sure about that.  I'm aware that we only map errors
below 3000 somewhere, but it's no safe bet that it stays that way.

For instance, we handle NT status codes STATUS_TRANSACTIONAL_CONFLICT
and STATUS_TRANSACTION_NOT_ENLISTED and those translate into the TxF
error range between 6800 and 6899.  We don't convert those to userspace
errno yet, but consider having to add them at one point and thus having
to add the 3000 entries from the last used one up the newly used one.

The reason to keep them is to allow us to be lazy about it.  The list
also just takes ~36K, and with the change to uint8_t it only takes
9K, so what?

> I could provide separate patches if desired.

Always welcome!


Thanks,
Corinna

Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[Christian Franke]

Corinna Vinschen wrote:
> On Feb 25 10:12, Christian Franke wrote:
>> Corinna Vinschen wrote:
>>> So the default was EPERM at first and has been changed to EACCES
>>> because it "is better for the unknown error case".
>>>
>>> I'm open to ideas for an improved error mapping.
>> I have no better suggestion for a default errno. Adding a cygwin specific
>> one (like ENMFILE, ENOSHARE and ECASECLASH added 2000-2001) is possibly not
>> desired.
> ENOSHARE and ECASECLASH are not used anymore, fortunately, and ENMFILE
> is hopefully never returned to userspace.  It might be a good idea to
> remove it from Cygwin's code as well.
>
>> Some thoughts about minor improvements of the errmap.h file:
>> - Add error number to each /* ERROR_... */ comment, e.g. /* 2:
>> ERROR_FILE_NOT_FOUND */.
> Ok.
>
>> - Update /* NUMBER */ comments using current MinGW-w64's winerror.h (~850
>> changes).
> Why so many?  I used winerror.h to populate the list not too long ago,
> so I wonder why it suddenly has so many more error codes?

"Required for mozilla-central." - 850 insertions:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ddeb05a

Most or all would possible never occur with the NTDLL/Win32 API subset 
used by Cygwin.

Includes interesting codes like "ERROR_NO_WORK_DONE" :-)


>> - Max errno is 143, so data type size could be reduced from int to uint8_t
>> aka unsigned char. Could even add a compile time check by using C++11's
>> braced initializers which do not allow narrowing conversions.
> Yeah, we could do that.
>
>> - Remove trailing entries which only map to 0.
>> - Append a static_assert which checks whether array size matches the last
>> mapped error number.
> Yeah, not so sure about that.  I'm aware that we only map errors
> below 3000 somewhere, but it's no safe bet that it stays that way.
>
> For instance, we handle NT status codes STATUS_TRANSACTIONAL_CONFLICT
> and STATUS_TRANSACTION_NOT_ENLISTED and those translate into the TxF
> error range between 6800 and 6899.  We don't convert those to userspace
> errno yet, but consider having to add them at one point and thus having
> to add the 3000 entries from the last used one up the newly used one.
>
> The reason to keep them is to allow us to be lazy about it.  The list
> also just takes ~36K, and with the change to uint8_t it only takes
> 9K, so what?

Ok.

>> I could provide separate patches if desired.
> Always welcome!

Ok.

Thanks,
Christian

Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV

[Corinna Vinschen]

On Feb 26 12:14, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Why so many?  I used winerror.h to populate the list not too long ago,
> > so I wonder why it suddenly has so many more error codes?
> 
> "Required for mozilla-central." - 850 insertions:
> https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ddeb05a
> 
> Most or all would possible never occur with the NTDLL/Win32 API subset used
> by Cygwin.
> 
> Includes interesting codes like "ERROR_NO_WORK_DONE" :-)

LOL.  The only hint I found on that one is in Wine ChangeLog:

 "FormatMessage() now reports ERROR_NO_WORK_DONE error for empty string."


Corinna