vmh: fix config pasing issue#8772
Conversation
Config was not correctly read by API. Fix config copying. Signed-off-by: Jakub Dabek <jakub.dabek@intel.com>
dabekjakub
force-pushed
the
vmh_heap_fix
branch
from
0b8b79d to
d510e67
Compare
|
Cannot fix checkpatch in a way it would pass - left best possible and readable way. |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
sorry, don't understand what this is fixing. What exactly was wrong with the existing version?
| cfg = &new_config; | ||
| } | ||
| else | ||
| memcpy_s(&new_config, sizeof(struct vmh_heap_config), |
There was a problem hiding this comment.
Old code resulted in the following behavior:
cfg was read correctly only in the if context and reads done after that returned garbage.
I do not understand the origin of the issue but sticking to new_config which was created and zeroed in the function body seems to fix the issue.
There was a problem hiding this comment.
can you give more context in the commit message, it looks like we are changing an object from heap ptr to stack ?
There was a problem hiding this comment.
Old code resulted in the following behavior: cfg was read correctly only in the if context and reads done after that returned garbage. I do not understand the origin of the issue but sticking to new_config which was created and zeroed in the function body seems to fix the issue.
@dabekjakub sorry, I have a substantial issue with fixing something that we don't understand, especially if that's our own open-source software, not some 3rd-party black box.
There was a problem hiding this comment.
Well, I agree but it is either this or the code not working. Which do we choose? This can be investigated further but this fixes heap creation.
Both ptr should be stack ptrs since cfg should be created runtime and not assigned space on the heap. At least that is an assumption I am making it is a client that will decide from where he will pass config.
https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html
Looks good enough to me. |
|
@lyakh Was right here. After another debug session narrowed it down to testing issue. Closing. |
4 participants
Config was not correctly read by API. Fix config copying.