| Message ID | 20220919084610.25420-1-linmq006@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | opal-prd: Fix missing cleanups in chip_init | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-Docker_builds_and_checks | success | Successfully ran 9 jobs. |
Hello, Sorry for the late reply. Your message got lost in the list's "pending moderator approval" queue. On Mon, Sep 19, 2022 at 12:46:10PM +0400, Miaoqian Lin wrote: >diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c >index 1c610da4c0b6..ed47260d23bd 100644 >--- a/external/opal-prd/opal-prd.c >+++ b/external/opal-prd/opal-prd.c >@@ -1290,22 +1290,28 @@ static int chip_init(void) > dirent->d_name); > if (rc < 0) { > pr_log(LOG_ERR, "FW: Failed to create chip-id path"); >+ closedir(dir); > return -1; > } > > rc = open_and_read(path, &buf, &len); > if (rc) { > pr_log(LOG_ERR, "FW; Failed to read chipid"); >+ closedir(dir); >+ free(path); > return -1; > } > chipid = buf; > chips[nr_chips++] = be32toh(*chipid); >+ free(path); Doesn't chipid/buf also need to be freed here? It is allocated by open_and_read(). > } > > pr_log(LOG_DEBUG, "FW: Chip init"); > for (i = 0; i < nr_chips; i++) > pr_log(LOG_DEBUG, "FW: Chip 0x%lx", chips[i]); > >+ closedir(dir); >+ > return 0; I think it would be more readable to replace the other closedir()+return pairs with 'goto' and a jump label here at the end: out_closedir: closedir(dir); return rc; > }
On 2023/1/4 5:19, Reza Arbab wrote: > Hello, > > Sorry for the late reply. Your message got lost in the list's "pending moderator approval" queue. > > On Mon, Sep 19, 2022 at 12:46:10PM +0400, Miaoqian Lin wrote: >> diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c >> index 1c610da4c0b6..ed47260d23bd 100644 >> --- a/external/opal-prd/opal-prd.c >> +++ b/external/opal-prd/opal-prd.c >> @@ -1290,22 +1290,28 @@ static int chip_init(void) >> dirent->d_name); >> if (rc < 0) { >> pr_log(LOG_ERR, "FW: Failed to create chip-id path"); >> + closedir(dir); >> return -1; >> } >> >> rc = open_and_read(path, &buf, &len); >> if (rc) { >> pr_log(LOG_ERR, "FW; Failed to read chipid"); >> + closedir(dir); >> + free(path); >> return -1; >> } >> chipid = buf; >> chips[nr_chips++] = be32toh(*chipid); >> + free(path); > > Doesn't chipid/buf also need to be freed here? It is allocated by open_and_read(). > The memory allocated by open_and_read() is stored as chipid/buf. It is stored in 'chips' array, which is a global variable and used in other places. I don't think we need to release it here. >> } >> >> pr_log(LOG_DEBUG, "FW: Chip init"); >> for (i = 0; i < nr_chips; i++) >> pr_log(LOG_DEBUG, "FW: Chip 0x%lx", chips[i]); >> >> + closedir(dir); >> + >> return 0; > > I think it would be more readable to replace the other closedir()+return pairs with 'goto' and a jump label here at the end: > Thanks for your review, I'll send v2 to fix this. > out_closedir: > closedir(dir); > return rc; > >> } >
On Thu, Jan 05, 2023 at 12:08:45AM +0800, Miaoqian Lin wrote: >On 2023/1/4 5:19, Reza Arbab wrote: >> On Mon, Sep 19, 2022 at 12:46:10PM +0400, Miaoqian Lin wrote: >>> diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c >>> index 1c610da4c0b6..ed47260d23bd 100644 >>> --- a/external/opal-prd/opal-prd.c >>> +++ b/external/opal-prd/opal-prd.c >>> @@ -1290,22 +1290,28 @@ static int chip_init(void) >>> dirent->d_name); >>> if (rc < 0) { >>> pr_log(LOG_ERR, "FW: Failed to create chip-id path"); >>> + closedir(dir); >>> return -1; >>> } >>> >>> rc = open_and_read(path, &buf, &len); >>> if (rc) { >>> pr_log(LOG_ERR, "FW; Failed to read chipid"); >>> + closedir(dir); >>> + free(path); >>> return -1; >>> } >>> chipid = buf; >>> chips[nr_chips++] = be32toh(*chipid); >>> + free(path); >> >> Doesn't chipid/buf also need to be freed here? It is allocated by open_and_read(). >> >The memory allocated by open_and_read() is stored as chipid/buf. It is stored in 'chips' array, > >which is a global variable and used in other places. I don't think we need to release it here. Unless I'm misunderstanding, chipid/buf is dereferenced, so the value it points to is copied by assignment to an element in the 'chips' array. After that, the memory is no longer needed.
diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c index 1c610da4c0b6..ed47260d23bd 100644 --- a/external/opal-prd/opal-prd.c +++ b/external/opal-prd/opal-prd.c @@ -1290,22 +1290,28 @@ static int chip_init(void) dirent->d_name); if (rc < 0) { pr_log(LOG_ERR, "FW: Failed to create chip-id path"); + closedir(dir); return -1; } rc = open_and_read(path, &buf, &len); if (rc) { pr_log(LOG_ERR, "FW; Failed to read chipid"); + closedir(dir); + free(path); return -1; } chipid = buf; chips[nr_chips++] = be32toh(*chipid); + free(path); } pr_log(LOG_DEBUG, "FW: Chip init"); for (i = 0; i < nr_chips; i++) pr_log(LOG_DEBUG, "FW: Chip 0x%lx", chips[i]); + closedir(dir); + return 0; }
1. The function calls opendir() but missing the corresponding closedir() before exit the function. 2. asprintf() alloc memory for path, we need to free it after use. Add missing closedir() and free() to fix it. Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- external/opal-prd/opal-prd.c | 6 ++++++ 1 file changed, 6 insertions(+)