Fix errno bugs #343
Fix errno bugs #343
Conversation
libkmod: make sure errno is not overwritten in libkmod-file shared: do not set errno in read_str_long and read_str_ulong tools: make sure errno is set correctly in modinfo Signed-off-by: Howard Huang <howard2.huang@intel.com>
evelikov
left a comment
evelikov
left a comment
There was a problem hiding this comment.
Thanks for tackling the issue I've opened. As the comments imply - please split this into separate commits (having it as one PR is fine) and cover the why in the commit messages.
|
|
||
| close(file->fd); | ||
| free(file); | ||
| errno = err; |
There was a problem hiding this comment.
I don't follow why we're deferring errno assignment after close/free. The man page for both, explicitly says that it's unchanged in case of non-error.
There was a problem hiding this comment.
Is it possible for close to fail and set errno? Or does it not matter because the file is opened with O_RDONLY?
There was a problem hiding this comment.
Technically EINTR and EIO are possible. I don't think we care/check for those elsewhere in the codebase.
It's should be fine to move the assignment as you did, but the commit message should mention why.
| err = read_str_safe(fd, buf, sizeof(buf)); | ||
| if (err < 0) | ||
| return err; | ||
| errno = 0; |
There was a problem hiding this comment.
The commit message should say why it's safe to remove this... Seemingly because it's a) not part of the API and b) unused/unchecked.
| it = malloc(sizeof(struct param)); | ||
| if (it == NULL) | ||
| if (it == NULL) { | ||
| errno = ENOMEM; |
There was a problem hiding this comment.
This is bonkers - malloc already sets errno. On the other hand, the add_param() caller, does not need the struct instance, so we can return the errno directlystatic int add_param(...). Is there anything preventing us from doing that here?
Looking at the code-path... We could also pass the type (parm vs parmtype) as an argument to process_parm(), remove the duplicate streq() call within and pass a single data/datalen (instead of {param,type}{,len}) to add_param(). But that can happen with later patch/MR.
|
Fwiw I've opened #346 which seems like a more complete solution. Do you mind having a look - chances are, I may have done something silly within 😅 Thanks |
|
Issue should be resolved with #346, so let's close this PR. |
2 participants
libkmod: make sure errno is not overwritten in libkmod-file
shared: do not set errno in read_str_long and read_str_ulong
tools: make sure errno is set correctly in modinfo
Closes: #244