| Message ID | 20190102131318.5765-16-honli@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ibsim,01/23] move sim_cmd_file into ibsim/sim_cmd.c | expand |
On 1/2/2019 8:13 AM, Honggang Li wrote: > Issue was detected by Coverity. > --------------------------- > ibsim-0.7/umad2sim/umad2sim.c:151: check_return: Calling "mkdir(dir, 493U)" without checking return value. This library function may fail and return an error code. > --------------------------- > > Also covert the function into a void function, as none of callers > checked the return value of make_path. > > Signed-off-by: Honggang Li <honli@redhat.com> > --- > umad2sim/umad2sim.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/umad2sim/umad2sim.c b/umad2sim/umad2sim.c > index 84ab84fd81b6..80e7b8166638 100644 > --- a/umad2sim/umad2sim.c > +++ b/umad2sim/umad2sim.c > @@ -137,7 +137,7 @@ static void convert_sysfs_path(char *new_path, unsigned size, > snprintf(new_path, size, "%s/%s", umad2sim_sysfs_prefix, old_path); > } > > -static int make_path(char *path) > +static void make_path(char *path) > { > char dir[1024]; > char *p; > @@ -148,14 +148,13 @@ static int make_path(char *path) > p = strchr(p, '/'); > if (p) > *p = '\0'; > - mkdir(dir, 0755); > + if (mkdir(dir, 0755) && errno != EEXIST) Why not warn when errno is EEXIST ? > + IBWARN("Failed to make directory <%s>", dir); While this change silences Coverity, I'm not sure that things should continue in face of such an error as things will not be setup correctly. > if (p) { > *p = '/'; > p++; > } > } while (p && p[0]); > - > - return 0; > } > > static int file_printf(char *path, char *name, const char *fmt, ...) >
On Tue, Jan 08, 2019 at 09:00:48AM -0500, Hal Rosenstock wrote: > On 1/2/2019 8:13 AM, Honggang Li wrote: > > Issue was detected by Coverity. > > --------------------------- > > ibsim-0.7/umad2sim/umad2sim.c:151: check_return: Calling "mkdir(dir, 493U)" without checking return value. This library function may fail and return an error code. > > --------------------------- > > > > Also covert the function into a void function, as none of callers > > checked the return value of make_path. > > > > Signed-off-by: Honggang Li <honli@redhat.com> > > --- > > umad2sim/umad2sim.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/umad2sim/umad2sim.c b/umad2sim/umad2sim.c > > index 84ab84fd81b6..80e7b8166638 100644 > > --- a/umad2sim/umad2sim.c > > +++ b/umad2sim/umad2sim.c > > @@ -137,7 +137,7 @@ static void convert_sysfs_path(char *new_path, unsigned size, > > snprintf(new_path, size, "%s/%s", umad2sim_sysfs_prefix, old_path); > > } > > > > -static int make_path(char *path) > > +static void make_path(char *path) > > { > > char dir[1024]; > > char *p; > > @@ -148,14 +148,13 @@ static int make_path(char *path) > > p = strchr(p, '/'); > > if (p) > > *p = '\0'; > > - mkdir(dir, 0755); > > + if (mkdir(dir, 0755) && errno != EEXIST) > > Why not warn when errno is EEXIST ? I instrumented the code to dump some messages for debugging. make_path creates the directory from the root to leaf. If two path shall same parent directory, the second call to 'mkdir' of parent directory will be failed with errno set to 'EEXIST'. make_path(/sys/class/infiniband_mad) call mkdir(., 0755) = -1, errno = EEXIST make_path(/sys/class/infiniband_mad) call mkdir(./sys-15302, 0755) = 0 make_path(/sys/class/infiniband_mad) call mkdir(./sys-15302/, 0755) = -1, errno = EEXIST make_path(/sys/class/infiniband_mad) call mkdir(./sys-15302//sys, 0755) = 0 ^^^^^^^^^^^^^^^ make_path(/sys/class/infiniband_mad) call mkdir(./sys-15302//sys/class, 0755) = 0 make_path(/sys/class/infiniband_mad) call mkdir(./sys-15302//sys/class/infiniband_mad, 0755) = 0 make_path(/sys/class/infiniband/ibsim0) call mkdir(., 0755) = -1, errno = EEXIST make_path(/sys/class/infiniband/ibsim0) call mkdir(./sys-15302, 0755) = -1, errno = EEXIST make_path(/sys/class/infiniband/ibsim0) call mkdir(./sys-15302/, 0755) = -1, errno = EEXIST make_path(/sys/class/infiniband/ibsim0) call mkdir(./sys-15302//sys, 0755) = -1, errno = EEXIST ^^^^^^^^^^^^^^^^^ make_path(/sys/class/infiniband/ibsim0) call mkdir(./sys-15302//sys/class, 0755) = -1, errno = EEXIST make_path(/sys/class/infiniband/ibsim0) call mkdir(./sys-15302//sys/class/infiniband, 0755) = 0 make_path(/sys/class/infiniband/ibsim0) call mkdir(./sys-15302//sys/class/infiniband/ibsim0, 0755) = 0 > > > + IBWARN("Failed to make directory <%s>", dir); > > While this change silences Coverity, I'm not sure that things should > continue in face of such an error as things will not be setup correctly. Yes, you are right. Should replace 'IBWARN' with 'IBPANIC'. [ibsim patch v2] umad2sim.c: make_path should check the return value I sent the new patch for review. > > > if (p) { > > *p = '/'; > > p++; > > } > > } while (p && p[0]); > > - > > - return 0; > > } > > > > static int file_printf(char *path, char *name, const char *fmt, ...) > >
diff --git a/umad2sim/umad2sim.c b/umad2sim/umad2sim.c index 84ab84fd81b6..80e7b8166638 100644 --- a/umad2sim/umad2sim.c +++ b/umad2sim/umad2sim.c @@ -137,7 +137,7 @@ static void convert_sysfs_path(char *new_path, unsigned size, snprintf(new_path, size, "%s/%s", umad2sim_sysfs_prefix, old_path); } -static int make_path(char *path) +static void make_path(char *path) { char dir[1024]; char *p; @@ -148,14 +148,13 @@ static int make_path(char *path) p = strchr(p, '/'); if (p) *p = '\0'; - mkdir(dir, 0755); + if (mkdir(dir, 0755) && errno != EEXIST) + IBWARN("Failed to make directory <%s>", dir); if (p) { *p = '/'; p++; } } while (p && p[0]); - - return 0; } static int file_printf(char *path, char *name, const char *fmt, ...)
Issue was detected by Coverity. --------------------------- ibsim-0.7/umad2sim/umad2sim.c:151: check_return: Calling "mkdir(dir, 493U)" without checking return value. This library function may fail and return an error code. --------------------------- Also covert the function into a void function, as none of callers checked the return value of make_path. Signed-off-by: Honggang Li <honli@redhat.com> --- umad2sim/umad2sim.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)