Hi Yonghong

On 01/18/21 09:48, Yonghong Song wrote:
> The original patch code:
> 
> +static int trigger_module_test_write(int write_sz)
> +{
> +     int fd, err;
> +     char *buf = malloc(write_sz);
> +
> +     if (!buf)
> +             return -ENOMEM;
> +
> +     memset(buf, 'a', write_sz);
> +     buf[write_sz-1] = '\0';
> +
> +     fd = open("/sys/kernel/bpf_testmod", O_WRONLY);
> +     err = -errno;
> +     if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
> +             goto out;
> +
> +     write(fd, buf, write_sz);
> +     close(fd);
> +out:
> +     free(buf);
> +
> +     return 0;
> +}
> 
> Even for "fd < 0" case, it "goto out" and "return 0". We should return
> error code here instead of 0.
> 
> Second, "err = -errno" is set before checking fd < 0. If fd >= 0, err might
> inherit an postive errno from previous failure.
> In trigger_module_test_write(), it is okay since the err is only used
> when fd < 0:
>         err = -errno;
>         if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
>                 return err;
> 
> My above rewrite intends to use "err" during final "return" statement,
> so I put assignment of "err = -errno" inside the CHECK branch.
> But there are different ways to implement this properly.

Okay I see now. Sorry I missed your point initially. I will fix and send v3.

Thanks

--
Qais Yousef

Reply via email to