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
