Returning FrameAlreadyMapped error when it shouldn't

[Sxmourai]

The MapToError::FrameAlreadyMapped is returned when we try to map a frame that is already mapped.
From the offset_page_table -> Size4Kib code:

if !p1[page.p1_index()].is_unused() {
      return Err(MapToError::PageAlreadyMapped(frame));
}

We get this error when we try to change the page flags, which isn't right, we should only return this error if the frame address's are different.
Something like:

if !p1[page.p1_index()].is_unused() && p1[page.p1_index()].addr() != frame.start_address() {
     return Err(MapToError::PageAlreadyMapped(frame));
}

Also why do we return the user inputted frame and not the previously mapped frame ?

[Freax13]

We get this error when we try to change the page flags, which isn't right, we should only return this error if the frame address's are different.

Is there a reason why you aren't using Mapper::update_flags if you only want to change the flags and not the frame?

Also why do we return the user inputted frame and not the previously mapped frame ?

See #117 and #118.

[Sxmourai]

Sorry, I didn't see this function !
But maybe we should use the update_flags implicitly ?

[Freax13]

I think there are some users of the mapping function that expect it to return an error if the entry is already present even if that entry contains the same frame. Changing this behavior would be a breaking change and a subtle one.

[Sxmourai]

Yes I'm okay, but also why do we have Mapper::update_flags but not Mapper::get_flags, I think it would be a cool addition

[Freax13]

Yes I'm okay, but also why do we have Mapper::update_flags but not Mapper::get_flags, I think it would be a cool addition

That sounds reasonable to me. Feel free to open a PR :)

[phil-opp]

Yes I'm okay, but also why do we have Mapper::update_flags but not Mapper::get_flags, I think it would be a cool addition

The Mapper trait is about updating the page tables, i.e. creating or changing some mappings. We have a separate trait called Translate for inspecting the page tables without modification. For example, you can use the Translate::translate to get all the information about a virtual page, including its flags.

[Sxmourai]

Okay thanks !
Well I guess my issue is useless... So I can close it...
Although I think it's not really straight forward...

[phil-opp]

Although I think it's not really straight forward...

Do you think it's mainly a documentation issue or do you see a way how we could make the API itself simpler?

[Sxmourai]

The API should change, because I think the update_flags and get_flags should be on the same object
But we can also just add in comments for the update_flags function:
/// See Translate::get_flags to get the flags on a specific page/frame

[phil-opp]

You can check the docs for a specific page table type to see all supported methods, e.g. https://docs.rs/x86_64/latest/x86_64/structures/paging/mapper/struct.OffsetPageTable.html. Keeping the traits separate allows creating custom read-only or write-only page table types.

But we can also just add in comments for the update_flags function:
/// See Translate::get_flags to get the flags on a specific page/frame

I agree that such a comment would be useful. Could you submit a PR?