| Message ID | 20210806032131.25721-1-zhanglianjie@uniontech.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib/tst_tmpdir: tst_get_tmpdir() add error handing | expand |
Hi On 8/6/2021 5:21 AM, zhanglianjie wrote: > Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com> > > diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c > index 0c39eb89f..f006e4893 100644 > --- a/lib/tst_tmpdir.c > +++ b/lib/tst_tmpdir.c > @@ -108,12 +108,18 @@ int tst_tmpdir_created(void) > > char *tst_get_tmpdir(void) > { > + char *ret = NULL; > + > if (TESTDIR == NULL) { > tst_brkm(TBROK, NULL, "you must call tst_tmpdir() first"); > return NULL; > } > > - return strdup(TESTDIR); > + ret = strdup(TESTDIR); Is a failing strdup here really a thing? The only reason strdup should be able to fail is with ENOMEM. The only way tst_brkm will work, if strdup fails here is, if TESTDIR is an extremely huge string (the NULL case is already handled above). > + if (!ret) > + tst_brkm(TBROK, NULL, "strdup() failed"); > + > + return ret; > } > > const char *tst_get_startwd(void) > -- > 2.20.1 > > Joerg
Hi, > On 8/6/2021 5:21 AM, zhanglianjie wrote: >> Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com> >> >> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c >> index 0c39eb89f..f006e4893 100644 >> --- a/lib/tst_tmpdir.c >> +++ b/lib/tst_tmpdir.c >> @@ -108,12 +108,18 @@ int tst_tmpdir_created(void) >> >> char *tst_get_tmpdir(void) >> { >> + char *ret = NULL; >> + >> if (TESTDIR == NULL) { >> tst_brkm(TBROK, NULL, "you must call tst_tmpdir() first"); >> return NULL; >> } >> >> - return strdup(TESTDIR); >> + ret = strdup(TESTDIR); > Is a failing strdup here really a thing? The only reason strdup should > be able to fail is with ENOMEM. > The only way tst_brkm will work, if strdup fails here is, if TESTDIR is > an extremely huge string (the NULL case is already handled above). If don’t consider a very huge string, you don’t need to modify it here. This is a habitual judgment. Thank you for your review. >> + if (!ret) >> + tst_brkm(TBROK, NULL, "strdup() failed"); >> + >> + return ret; >> } >> >> const char *tst_get_startwd(void) >> -- >> 2.20.1 >> >> > Joerg >
Hi! > > - return strdup(TESTDIR); > > + ret = strdup(TESTDIR); > Is a failing strdup here really a thing? The only reason strdup should > be able to fail is with ENOMEM. > The only way tst_brkm will work, if strdup fails here is, if TESTDIR is > an extremely huge string (the NULL case is already handled above). It's unlikely, but it may happen if: * Someone runs memeater on the baground along with LTP * And kernel is set not to overcommit And in a case of the test lirary I would rather see it written so that we check everything, even the unlikely errors.
Hi! Pushed, thanks.
diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c index 0c39eb89f..f006e4893 100644 --- a/lib/tst_tmpdir.c +++ b/lib/tst_tmpdir.c @@ -108,12 +108,18 @@ int tst_tmpdir_created(void) char *tst_get_tmpdir(void) { + char *ret = NULL; + if (TESTDIR == NULL) { tst_brkm(TBROK, NULL, "you must call tst_tmpdir() first"); return NULL; } - return strdup(TESTDIR); + ret = strdup(TESTDIR); + if (!ret) + tst_brkm(TBROK, NULL, "strdup() failed"); + + return ret; } const char *tst_get_startwd(void)
Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com> -- 2.20.1