> Date: Sat, 13 Apr 2024 12:37:41 +0200
> From: Pali <pali@pali.im>
> Cc: djgpp@delorie.com
>
> Hello, I have there a change for __djgpp_set_page_attributes() function
> which sets more sane errno value on different failures. Could you look
> at it if it is useful?

Thanks. I think this can be useful, but I'd prefer to keep the style
of having just one exit point in this function. So could you please
rewrite the patch so that the various conditions only set errno, and
still "goto fail"?

> Date: Sat, 13 Apr 2024 14:01:09 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> On Saturday 13 April 2024 14:51:17 Eli Zaretskii wrote:
> > > Date: Sat, 13 Apr 2024 12:37:41 +0200
> > > From: Pali <pali@pali.im>
> > > Cc: djgpp@delorie.com
> > >
> > > Hello, I have there a change for __djgpp_set_page_attributes() function
> > > which sets more sane errno value on different failures. Could you look
> > > at it if it is useful?
> >
> > Thanks. I think this can be useful, but I'd prefer to keep the style
> > of having just one exit point in this function. So could you please
> > rewrite the patch so that the various conditions only set errno, and
> > still "goto fail"?
>
> Ok. Something like this?

Yes, but:

>
> --- src/libc/dpmi/helper/setattr.c
> +++ src/libc/dpmi/helper/setattr.c
> @@ -28,7 +28,7 @@ __djgpp_set_page_attributes (void *_our_addr, unsigned long _num_bytes,
> || ((_attributes & 0x3) == 2))
> {
> errno = EINVAL;
> - return -1;
> + goto fail;
> }
>
> /* Set up an array of page attribute information. */
> @@ -50,7 +50,17 @@ __djgpp_set_page_attributes (void *_our_addr, unsigned long _num_bytes,
> /* Find the memory handle corresponding to the first byte. */
> d = __djgpp_memory_handle (p);
> if (d == NULL)
> - goto fail;
> + {
> + errno = EFAULT;
> + goto fail;
> + }
> +
> + /* Base address of the memory handle must be page aligned too. */
> + if (d->address & 0xfff)
> + {
> + errno = EFAULT;
> + goto fail;
> + }
>
> /* Find the last byte in the range that's also in the same
> * memory handle as our current starting byte. We start with
> @@ -68,7 +78,10 @@ __djgpp_set_page_attributes (void *_our_addr, unsigned long _num_bytes,
> /* Find the memory handle corresponding to this test byte. */
> d2 = __djgpp_memory_handle (handle_end_addr);
> if (d2 == NULL)
> - goto fail;
> + {
> + errno = EFAULT;
> + goto fail;
> + }
>
> /* Is this test byte in the same handle as the first byte? */
> if (d2->handle == d->handle)
> @@ -81,9 +94,35 @@ __djgpp_set_page_attributes (void *_our_addr, unsigned long _num_bytes,
> meminfo.handle = d->handle;
> meminfo.size = num_pages2;
> meminfo.address = p - d->address;
> - if (__dpmi_set_page_attributes (&meminfo, attr)
> - || meminfo.size != num_pages2)
> - goto fail;
> + if (__dpmi_set_page_attributes (&meminfo, attr))
> + {
> + switch (__dpmi_error)
> + {
> + case 0x0507: /* Unsupported function (returned by DPMI 0.9 host, error number is same as DPMI function number) */
> + case 0x8001: /* Unsupported function (returned by DPMI 1.0 host) */
> + errno = ENOSYS;
> + break;
> + case 0x8002: /* Invalid state (page in wrong state for request) */
> + errno = EFAULT;
> + break;
> + case 0x8010: /* Resource unavailable (DPMI host cannot allocate internal resources to complete an operation) */
> + case 0x8013: /* Physical memory unavailable */
> + case 0x8014: /* Backing store unavailable */
> + errno = ENOMEM;
> + break;
> + case 0x8021: /* Invalid value (illegal request in bits 0-2 of one or more page attribute words) */
> + errno = EINVAL;
> + break;
> + case 0x8023: /* Invalid handle (in ESI) */
> + case 0x8025: /* Invalid linear address (specified range is not within specified block) */
> + errno = EFAULT;
> + break;
> + default: /* Other unspecified error */
> + errno = EIO;
> + break;

Why EIO and not the original EACCES?

Also, can you collect all EFAULT cases in the switch together?

On 2024-04-13 13:51, Eli Zaretskii (eliz@gnu.org) [via djgpp@delorie.com] wrote:
>> Date: Sat, 13 Apr 2024 12:37:41 +0200
>> From: Pali <pali@pali.im>
>> Cc: djgpp@delorie.com

Just wanted to point out, I haven't received any of Pali's messages on
this list. They also don't appear in the archives:

https://www.delorie.com/djgpp/mail-archives/browse.cgi?p=djgpp/2024/04/13

Not sure what's going wrong there.

> Date: Sat, 13 Apr 2024 16:59:29 +0200
> From: "J.W. Jagersma (jwjagersma@gmail.com) [via djgpp@delorie.com]" <djgpp@delorie.com>
>
> On 2024-04-13 13:51, Eli Zaretskii (eliz@gnu.org) [via djgpp@delorie.com] wrote:
> >> Date: Sat, 13 Apr 2024 12:37:41 +0200
> >> From: Pali <pali@pali.im>
> >> Cc: djgpp@delorie.com
>
> Just wanted to point out, I haven't received any of Pali's messages on
> this list. They also don't appear in the archives:
>
> https://www.delorie.com/djgpp/mail-archives/browse.cgi?p=djgpp/2024/04/13
>
> Not sure what's going wrong there.

DJ, is there something wrong with the list server?

> Date: Sat, 13 Apr 2024 14:23:45 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> On Saturday 13 April 2024 15:10:34 Eli Zaretskii wrote:
> > > + default: /* Other unspecified error */
> > > + errno = EIO;
> > > + break;
> >
> > Why EIO and not the original EACCES?
>
> EACCES is "Permission denied" error.

I believe it's "Access denied" ("Permission denied" is EPERM).

EACCES was what we returned previously, isn't it? So why is it bad to
fall back on it? EIO is basically inappropriate at least the same as
EACCES: there's no "I/O" here, right?

> > Also, can you collect all EFAULT cases in the switch together?
>
> I sorted cases by dpmi error values. But EFAULT cases can be grouped
> easily, for example by moving case 0x8002 above case 0x8023.

Yes, please.

> Date: Sun, 14 Apr 2024 00:57:44 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> On Saturday 13 April 2024 18:54:04 Eli Zaretskii wrote:
> > > Date: Sat, 13 Apr 2024 14:23:45 +0200
> > > From: Pali <pali@pali.im>
> > > Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
> > >
> > > On Saturday 13 April 2024 15:10:34 Eli Zaretskii wrote:
> > > > > + default: /* Other unspecified error */
> > > > > + errno = EIO;
> > > > > + break;
> > > >
> > > > Why EIO and not the original EACCES?
> > >
> > > EACCES is "Permission denied" error.
> >
> > I believe it's "Access denied" ("Permission denied" is EPERM).
>
> EACCES is "Permission denied" error. Now I checked it via
> strerror(EACCES) call on both DJGPP and Linux runtimes.
>
> EPERM is "Operation not permitted" error.
>
> But this is quite interesting, I was looking at differences between
> EACCES and EPERM. Here are links to POSIX and GNU definitions:
>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03
> https://www.gnu.org/software/libc/manual/html_node/Error-Codes.html
>
> My understanding is that EACCES is an error caused by rwx file access
> permission check (e.g. opening file for writing which is marked as
> read-only, without w bit) and EPERM is an error caused by limited
> ownership (e.g. killing process of other user).
>
> So in my opinion both EACCES and EPERM are not good choices for this
> case of "other unspecified error".

EACCES is the DOS and Windows "generic" errno value, and is ubiquitous
on those two systems. Reading Posix references will not tell you the
whole story, since the "native" error codes on DOS and Windows are
quite different, and converting them to Posix codes is not always
trivial. That is why we see EACCES so much, and I believe that's why
the original code used it.

> > EACCES was what we returned previously, isn't it? So why is it bad to
> > fall back on it?
>
> Because I think that "Permission denied" error is misleading.
>
> > EIO is basically inappropriate at least the same as
> > EACCES: there's no "I/O" here, right?
>
> I see what you mean. I thought that EIO could be used for some generic
> error when changing page attributes, as it is doing some I/O operations
> on page tables. But I really do not know now. I/O term is mostly used
> when doing some kind of disk I/O operation.

I still think falling back on EACCES is the best way here. Since
every error that we can reasonably interpret now has its correct errno
value, what happens in the default case is really not important, and
leaving the default as it was before is slightly more backward
compatible, IMO.

> Date: Sun, 14 Apr 2024 10:29:32 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> Here is the modification which returns EACCES as generic error and move
> EFAULT errors into one case. I hope that everything is fixed now.

Thanks, installed.

> Date: Sun, 14 Apr 2024 12:03:31 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> I would propose to also update documentation for this function
> __djgpp_set_page_attributes to include all important information.

I think it's too much detail. We never describe errno values in such
detail anywhere else, do we? It's also a maintenance burden:
"Someone" will need to make sure the documentation is updated when the
code changes.

Why did you think this was important to spell out?

> Date: Sun, 14 Apr 2024 12:42:51 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> What I think is important: include information that out_addr/num_bytes
> address range must belong to sbrk allocator. This is something hard to
> figure out without reading the source code of the function.

What other allocator there can be in DJGPP? I thought all allocations
go through sbrk, isn't that so?

> Date: Sun, 14 Apr 2024 14:10:34 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> On Sunday 14 April 2024 14:46:02 Eli Zaretskii wrote:
> > > Date: Sun, 14 Apr 2024 12:42:51 +0200
> > > From: Pali <pali@pali.im>
> > > Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
> > >
> > > What I think is important: include information that out_addr/num_bytes
> > > address range must belong to sbrk allocator. This is something hard to
> > > figure out without reading the source code of the function.
> >
> > What other allocator there can be in DJGPP? I thought all allocations
> > go through sbrk, isn't that so?
>
> __djgpp_set_page_attributes function takes our_addr argument and it was
> not clear for me what it is. Current documentation refers to the DPMI
> 0507H function, which takes memory handle and offset in it, which
> describes linear memory. __djgpp_set_page_attributes on the other hand
> expects for that out_addr is the offset in the DS segment and is doing
> translation to memory handle + offset by looking into sbrk allocator
> (via _djgpp_memory_handle).

So I think it's important to say that the OUT_ADDR is the offset from
the DS segment base, and use 'sbrk' and 'malloc' as examples of
functions that return such addresses, in contrast to
__dpmi_allocate_linear_memory and similar APIs. IOW, say that offsets
from DS are the requirement, and use 'sbrk' etc. as examples of
satisfying the requirement. The fact that __djgpp_set_page_attributes
translates to linear addresses is also worth mentioning.

> Important is that it is from allocator (not from any other source).

AFAIU, this is not a requirement. For example, if I know that offset
0x1000 from DS belongs to the program's address space (as it usually
is), I can use that as OUT_ADDR. Right?

> Date: Sun, 14 Apr 2024 15:49:15 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> > So I think it's important to say that the OUT_ADDR is the offset from
> > the DS segment base, and use 'sbrk' and 'malloc' as examples of
> > functions that return such addresses, in contrast to
> > __dpmi_allocate_linear_memory and similar APIs. IOW, say that offsets
> > from DS are the requirement, and use 'sbrk' etc. as examples of
> > satisfying the requirement. The fact that __djgpp_set_page_attributes
> > translates to linear addresses is also worth mentioning.
> >
> > > Important is that it is from allocator (not from any other source).
> >
> > AFAIU, this is not a requirement. For example, if I know that offset
> > 0x1000 from DS belongs to the program's address space (as it usually
> > is), I can use that as OUT_ADDR. Right?
>
> If the memory offset 0x1000 from DS comes from the allocator (as it
> usually is, created during startup) then it would work. But if the
> offset, lets say 0x100000 from DS is behind the last byte allocated by
> sbrk then it would fail.
>
> It would fail because translation is done by _djgpp_memory_handle
> function and it works by looking into memory handles which were
> allocated by 'sbrk'.

But isn't the DS segment memory allocated via calls to 'sbrk', and
aren't its memory handles kept in __djgpp_memory_handle?

For example, when we read the executable code of a DJGPP program into
DPMI memory at startup, don't we allocate that memory via 'sbrk' and
store the handle in __djgpp_memory_handle?

> If you take uncommited memory via __dpmi_allocate_linear_memory function
> then it would not be from the allocator but still can be expressed as an
> offset from the DS.

I didn't mean just _any_ offset, I meant memory that is known to be
committed.

> Note that when __djgpp_nearptr_enable function was called and succeeded
> then any offset in 4GB address space is valid for DS, so also the memory
> obtained from the __dpmi_allocate_linear_memory function is
> automatically in the DS segment.

I didn't mean "fat DS", there's no need to take what I say ad
absurdum. All I meant to say was that the address can come in some
other way, not just via a direct call to 'sbrk' or 'malloc'.

> Anyway, similar documentation update was already done to the function
> __djgpp_map_physical_memory which basically uses same code pattern as
> __djgpp_set_page_attributes. And therefore has same requirements for
> our_addr/num_bytes parameters.

That could mean we should make the same changes there as well. Unless
I'm wrong in what I say above.

> Date: Sun, 14 Apr 2024 17:18:45 +0200
> From: Pali <pali@pali.im>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> What about this description? It is better?

Yes. But you haven't fixed the two earlier comments: about the
"address range" part and about @: after "e.g.".

> Date: Mon, 15 Apr 2024 18:04:37 +0300
> From: "Eli Zaretskii (eliz@gnu.org) [via djgpp@delorie.com]" <djgpp@delorie.com>
> Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
>
> > Date: Sun, 14 Apr 2024 17:18:45 +0200
> > From: Pali <pali@pali.im>
> > Cc: dj@delorie.com, sezeroz@gmail.com, djgpp@delorie.com
> >
> > What about this description? It is better?
>
> Yes. But you haven't fixed the two earlier comments: about the
> "address range" part and about @: after "e.g.".

Never mind, I fixed those myself and installed the changes.

Thanks.