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@xxxxxxxxxx>
> > ---
> > 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, ...)
> >
