Re: mmap fixes

[Ken Brown]

Hi Corinna,

On 12/19/2024 5:17 AM, Corinna Vinschen wrote:
Fixes: ??

Fixes: f90e23f2714cb ("*autoload.cc (NtCreateSection): Define.")

Darn the old CVS entries...

Thanks.  I don't know how you found that.

The patch looks good.  I just wonder if the new argument should be
called "new_prot" or "req_prot" to say a bit stronger that this
overrides the old prot...

Good idea.  I think I like "new_prot" better, since readers might not 
guess that "req" stands for "requested".

Anyway, LGTM.

OK, I'll push it with those changes.

I'm not quite sure with the second patch, though...

Fixes: ?

Fixes: c68de3a262fe5 ("* mmap.cc (class mmap_record): Declare new map_pages method 
with address parameter.")

Again thanks.  Some time you'll have to tell me how to find those commits.

-         if (u_addr > (caddr_t) addr || u_addr + len < (caddr_t) addr + len
+         if (u_addr > (caddr_t) addr || u_len < len
              || !rec->compatible_flags (flags))

While this is strictly correct, I wonder if this shouldn't be

   if (u_addr > (caddr_t) addr || u_addr + u_len < (caddr_t) addr + len ...

for plain readability.  The problem is, you can't see what match()
really returns, an intersection or the entire free region.  That's
what I stumbled over in the cygwin ML.

This way, the code immediately tells the reader that we want to make
sure that [addr,addr+len] is a region completely inside the region
[u_addr,u_addr+u_len], without needing to know what exactly match()
returns.  And it would still be correct, even if we redefine match().

What do you think?
I agree.  I'll make that change and push.  At some point it might be 
helpful to add a reference parameter "contains" to match(), so that 
match() can return the information about whether the mmap_record region 
contains [addr,addr+len].  That way the relevant tests can be done right 
where the reader can see what's going on.  But I'm not going to try to 
do that immediately.

Thanks for the review.

Ken