Skip to content

Conversation

@XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Jan 16, 2023

Summary

    1. rammap should return memory start address
    1. update output mapped pointer in file_mmap_ before returning

Impact

No.

Testing

mmap used in gettext now works correctly.

Signed-off-by: Xuxingliang <xuxingliang@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor

@jlaitine could you review the change? we found some regression from #8026

Copy link
Contributor

@jlaitine jlaitine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but there is another issue left, maybe look into it at the same?

entry->vaddr = rdbuffer;
entry->priv.i = kernel;
entry->munmap = unmap_rammap;

Copy link
Contributor

@jlaitine jlaitine Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "rdbuffer" pointer is no longer pointing to the original allocated pointer, when gotoed from lines 215, 245. In the "free" below on lines 253, 257 we should use entry->vaddr, since that stores the original allocated ptr

@jlaitine
Copy link
Contributor

Clarified my original comment, it was too quickly written to be understandable....

@XuNeo
Copy link
Contributor Author

XuNeo commented Jan 16, 2023

Let me update the commit later.

For int rammap(FAR struct file *filep, FAR struct mm_map_entry_s *entry, bool kernel), am I correct to add comment to say that entry->offset and entry->length must be initialized ?

Signed-off-by: Xuxingliang <xuxingliang@xiaomi.com>
Copy link
Contributor

@jlaitine jlaitine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@xiaoxiang781216
Copy link
Contributor

Let's ignore the unrelated ci broken which is fixed by #8155.

@xiaoxiang781216 xiaoxiang781216 merged commit 9166eed into apache:master Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants