| Message ID | 20221209110502.231677-1-liuxin350@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno Xin Liu | expand |
On 12/9/22 12:05 PM, Xin Liu wrote: > This is a small improvement in libbpf_strerror. When libbpf_strerror > is used to obtain the system error description, if the length of the > buf is insufficient, libbpf_sterror returns ERANGE and sets errno to > ERANGE. > > However, this processing is not performed when the error code > customized by libbpf is obtained. Make some minor improvements here, > return -ERANGE and set errno to ERANGE when buf is not enough for > custom description. nit: $subject line got corrupted? > Signed-off-by: Xin Liu <liuxin350@huawei.com> > --- > > v2: > Check the return value of snprintf to determine whether the buffer is > too small. > > v1: > https://lore.kernel.org/bpf/20221209084047.229525-1-liuxin350@huawei.com/T/#t > > tools/lib/bpf/libbpf_errno.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c > index 96f67a772a1b..6240c7cb7472 100644 > --- a/tools/lib/bpf/libbpf_errno.c > +++ b/tools/lib/bpf/libbpf_errno.c > @@ -39,14 +39,13 @@ static const char *libbpf_strerror_table[NR_ERRNO] = { > > int libbpf_strerror(int err, char *buf, size_t size) > { > + int ret; nit: newline after declaration > if (!buf || !size) > return libbpf_err(-EINVAL); > > err = err > 0 ? err : -err; > > if (err < __LIBBPF_ERRNO__START) { > - int ret; > - > ret = strerror_r(err, buf, size); > buf[size - 1] = '\0'; > return libbpf_err_errno(ret); > @@ -56,12 +55,20 @@ int libbpf_strerror(int err, char *buf, size_t size) > const char *msg; > > msg = libbpf_strerror_table[ERRNO_OFFSET(err)]; > - snprintf(buf, size, "%s", msg); > + ret = snprintf(buf, size, "%s", msg); > buf[size - 1] = '\0'; > + if (ret < 0) > + return libbpf_err_errno(ret); This would pass in ret == -1 and then eventually return 1 which is misleading, no? We have buf and msg non-NULL and a positive size, afaik, the only case where you could get a negative error now is when you pass in a buf with size exceeding INT_MAX.. > + if (ret >= size) > + return libbpf_err(-ERANGE); > return 0; > } > > - snprintf(buf, size, "Unknown libbpf error %d", err); > + ret = snprintf(buf, size, "Unknown libbpf error %d", err); > buf[size - 1] = '\0'; > + if (ret < 0) > + return libbpf_err_errno(ret); > + if (ret >= size) > + return libbpf_err(-ERANGE); > return libbpf_err(-ENOENT); > } >
On Sat, 10 Dec 2022 00:12:58 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > On 12/9/22 12:05 PM, Xin Liu wrote: > > This is a small improvement in libbpf_strerror. When libbpf_strerror > > is used to obtain the system error description, if the length of the > > buf is insufficient, libbpf_sterror returns ERANGE and sets errno to > > ERANGE. > > > > However, this processing is not performed when the error code > > customized by libbpf is obtained. Make some minor improvements here, > > return -ERANGE and set errno to ERANGE when buf is not enough for > > custom description. > > nit: $subject line got corrupted? > > > Signed-off-by: Xin Liu <liuxin350@huawei.com> > > --- > > > > v2: > > Check the return value of snprintf to determine whether the buffer is > > too small. > > > > v1: > > https://lore.kernel.org/bpf/20221209084047.229525-1-liuxin350@huawei.com/T/#t > > > > tools/lib/bpf/libbpf_errno.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c > > index 96f67a772a1b..6240c7cb7472 100644 > > --- a/tools/lib/bpf/libbpf_errno.c > > +++ b/tools/lib/bpf/libbpf_errno.c > > @@ -39,14 +39,13 @@ static const char *libbpf_strerror_table[NR_ERRNO] = { > > > > int libbpf_strerror(int err, char *buf, size_t size) > > { > > + int ret; > > nit: newline after declaration > > > if (!buf || !size) > > return libbpf_err(-EINVAL); > > > > err = err > 0 ? err : -err; > > > > if (err < __LIBBPF_ERRNO__START) { > > - int ret; > > - > > ret = strerror_r(err, buf, size); > > buf[size - 1] = '\0'; > > return libbpf_err_errno(ret); > > @@ -56,12 +55,20 @@ int libbpf_strerror(int err, char *buf, size_t size) > > const char *msg; > > > > msg = libbpf_strerror_table[ERRNO_OFFSET(err)]; > > - snprintf(buf, size, "%s", msg); > > + ret = snprintf(buf, size, "%s", msg); > > buf[size - 1] = '\0'; > > + if (ret < 0) > > + return libbpf_err_errno(ret); > > This would pass in ret == -1 and then eventually return 1 which > is misleading, no? > > We have buf and msg non-NULL and a positive size, afaik, the only > case where you could get a negative error now is when you pass in > a buf with size exceeding INT_MAX.. > > > + if (ret >= size) > > + return libbpf_err(-ERANGE); > > return 0; > > } > > > > - snprintf(buf, size, "Unknown libbpf error %d", err); > > + ret = snprintf(buf, size, "Unknown libbpf error %d", err); > > buf[size - 1] = '\0'; > > + if (ret < 0) > > + return libbpf_err_errno(ret); > > + if (ret >= size) > > + return libbpf_err(-ERANGE); > > return libbpf_err(-ENOENT); > > } > > The logic of returning negative numbers is really unlikely here, I'll add some comments and delete this logic, thanks to Daniel for pointing out.
diff --git a/tools/lib/bpf/libbpf_errno.c b/tools/lib/bpf/libbpf_errno.c index 96f67a772a1b..6240c7cb7472 100644 --- a/tools/lib/bpf/libbpf_errno.c +++ b/tools/lib/bpf/libbpf_errno.c @@ -39,14 +39,13 @@ static const char *libbpf_strerror_table[NR_ERRNO] = { int libbpf_strerror(int err, char *buf, size_t size) { + int ret; if (!buf || !size) return libbpf_err(-EINVAL); err = err > 0 ? err : -err; if (err < __LIBBPF_ERRNO__START) { - int ret; - ret = strerror_r(err, buf, size); buf[size - 1] = '\0'; return libbpf_err_errno(ret); @@ -56,12 +55,20 @@ int libbpf_strerror(int err, char *buf, size_t size) const char *msg; msg = libbpf_strerror_table[ERRNO_OFFSET(err)]; - snprintf(buf, size, "%s", msg); + ret = snprintf(buf, size, "%s", msg); buf[size - 1] = '\0'; + if (ret < 0) + return libbpf_err_errno(ret); + if (ret >= size) + return libbpf_err(-ERANGE); return 0; } - snprintf(buf, size, "Unknown libbpf error %d", err); + ret = snprintf(buf, size, "Unknown libbpf error %d", err); buf[size - 1] = '\0'; + if (ret < 0) + return libbpf_err_errno(ret); + if (ret >= size) + return libbpf_err(-ERANGE); return libbpf_err(-ENOENT); }
This is a small improvement in libbpf_strerror. When libbpf_strerror is used to obtain the system error description, if the length of the buf is insufficient, libbpf_sterror returns ERANGE and sets errno to ERANGE. However, this processing is not performed when the error code customized by libbpf is obtained. Make some minor improvements here, return -ERANGE and set errno to ERANGE when buf is not enough for custom description. Signed-off-by: Xin Liu <liuxin350@huawei.com> --- v2: Check the return value of snprintf to determine whether the buffer is too small. v1: https://lore.kernel.org/bpf/20221209084047.229525-1-liuxin350@huawei.com/T/#t tools/lib/bpf/libbpf_errno.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)