include/uk/plat/memory.h: Add check for memory region descriptor #797
Conversation
Make sure that the value of the memory region descriptor index is not bigger than the total count of memory region descriptors. Signed-off-by: guzman <daparrag@correo.udistrital.edu.co>
razvand
requested review from
mogasergiu and
RaduNichita
and removed request for
a team
|
@RaduNichita, @mogasergiu, please review this PR. It supersedes PR #771. |
|
@Daparrag, in the future, simply update the commits in the open PR by using |
| @@ -190,7 +190,7 @@ static struct uk_alloc *heap_init() | |||
| (void *)md->vbase, (void *)(md->vbase + md->len), | |||
| md->flags, | |||
| #if CONFIG_UKPLAT_MEMRNAME | |||
| md->name | |||
| md->name, | |||
There was a problem hiding this comment.
Why did you add a comma here?
| @@ -156,6 +161,9 @@ int ukplat_memregion_get(int i, struct ukplat_memregion_desc **m) | |||
| int ret; | |||
|
|
|||
| UK_ASSERT(m); | |||
There was a problem hiding this comment.
Add a newline between the UK_ASSERT and the if statement, I think it would look better.
| @@ -127,10 +127,13 @@ static int __linuxu_plat_initrd_init(void) | |||
|
|
|||
| int ukplat_memregion_count(void) | |||
| { | |||
| static int init = 0; | |||
| static int have_heap = 0; | |||
| static int have_initrd = 0; | |||
There was a problem hiding this comment.
Also, I believe that my example from the previously closed PR dropped the static keyword from have_heap and have_initrd. Why do you think they are still needed? As far as I can tell, static for init would be enough.
There was a problem hiding this comment.
Why do we keep these variables? I think it would be cleaner if we use only the rc and remove both have_heap and have_initrd
There was a problem hiding this comment.
Yeah, agreed, you are right. Something like this
int ukplat_memregion_count(void)
{
static int init;
if (init)
return init;
init = !__linuxu_plat_heap_init() + !__linuxu_plat_initrd_init();
return init;
}could work as well and you can drop everything but init 
.
Or
int ukplat_memregion_count(void)
{
static int init;
int rc;
if (init)
return init;
rc = __linuxu_plat_heap_init();
init += (rc == 0) ? 1 : 0 ;
rc = __linuxu_plat_initrd_init();
init += (rc == 0) ? 1 : 0 ;
return init;
}and the list could go on 😆 .
I don't think it matters that much in the end, though. @Daparrag choose whichever you like, as long as it works, looks decent and does not have any unnecessary statements (I would personally go with the latter).
There was a problem hiding this comment.
Good Thank you so much for your review and I will start working on that.
| @@ -127,10 +127,13 @@ static int __linuxu_plat_initrd_init(void) | |||
|
|
|||
| int ukplat_memregion_count(void) | |||
There was a problem hiding this comment.
Oh and one more thing. Based on your commit description, the change to ukplat_memregion_count should be in a separate commit, as it does not fix anything, but rather just adds a minor improvement. So, I would suggest you place this in a commit with a message like Avoid re-init of heap and initrd in ukplat_memregion_count or something like that and the actual fix with your current message in a separate commit. This way, this PR should have 2 commits instead of one: a commit with your current message and the changes done to ukplat_memregion_get (your actual fix) and a commit with your changes to ukplat_memregion_count (the improvement). This is how I would see it, just my opinion ;).
There was a problem hiding this comment.
I agree with @mogasergiu
| @@ -127,10 +127,13 @@ static int __linuxu_plat_initrd_init(void) | |||
|
|
|||
| int ukplat_memregion_count(void) | |||
| { | |||
| static int init = 0; | |||
| static int have_heap = 0; | |||
| static int have_initrd = 0; | |||
There was a problem hiding this comment.
Why do we keep these variables? I think it would be cleaner if we use only the rc and remove both have_heap and have_initrd
| @@ -127,10 +127,13 @@ static int __linuxu_plat_initrd_init(void) | |||
|
|
|||
| int ukplat_memregion_count(void) | |||
There was a problem hiding this comment.
I agree with @mogasergiu
|
@Daparrag, please see the reviews, update the PR, so we can then merge it. |
|
Solved by #878 |
Prerequisite checklist
checkpatch.plon your commit series before opening this PR;Base target
hellowordor N/A]Additional configuration
none
Description of changes
according to #766 the helloworld example was broken because an invalid memory allocator.
This problem occurred because you attempted to access a page without first determining the total number of available pages. To clarify further, the function ukplat_memregion_find_next requires the count of memory regions to be validated before calling the ukplat_memregion_find_next function to obtain the desired memory region.
For this reason I add a sanity check count = ukplat_memregion_count(); that will avoid the program to advance if the memory region is greater or equal than memory region count.
With this changes the hello world works as expected
---output after fixing
#766