| Message ID | 20180613194304.31548-2-aconole@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] lldp: fix string warnings | expand |
Looks good to me, thanks. Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> On Wed, Jun 13, 2018 at 12:43 PM, Aaron Conole <aconole@redhat.com> wrote: > lib/netdev-dpdk.c: In function : > lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the > last format character [-Wformat-truncation=] > snprintf(vhost_vring, 16, "vring_%d_size", i); > ^ > lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a > destination of size 16 > snprintf(vhost_vring, 16, "vring_%d_size", i); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Since vring_num is 16 bits, the largest value ever would only be 17 bytes, > including the terminating nul. Stretch it to 18 bytes (as a precaution > against a signed value, which again would never happen). > > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > lib/netdev-dpdk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 2e2f568b8..e75943bb2 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct > netdev *netdev, > > for (int i = 0; i < vring_num; i++) { > struct rte_vhost_vring vring; > - char vhost_vring[16]; > + char vhost_vring[18]; > > rte_vhost_get_vhost_vring(vid, i, &vring); > snprintf(vhost_vring, 16, "vring_%d_size", i); > -- > 2.14.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jun 13, 2018 at 03:43:04PM -0400, Aaron Conole wrote: > lib/netdev-dpdk.c: In function : > lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last format character [-Wformat-truncation=] > snprintf(vhost_vring, 16, "vring_%d_size", i); > ^ > lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a destination of size 16 > snprintf(vhost_vring, 16, "vring_%d_size", i); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Since vring_num is 16 bits, the largest value ever would only be 17 bytes, > including the terminating nul. Stretch it to 18 bytes (as a precaution > against a signed value, which again would never happen). > > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > lib/netdev-dpdk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 2e2f568b8..e75943bb2 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, > > for (int i = 0; i < vring_num; i++) { > struct rte_vhost_vring vring; > - char vhost_vring[16]; > + char vhost_vring[18]; > > rte_vhost_get_vhost_vring(vid, i, &vring); > snprintf(vhost_vring, 16, "vring_%d_size", i); Thanks for the improvement. Instead of calculating at all, I think it would be less error-prone to avoid it, something like this: diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index fd496592ba56..040b17f8a34a 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2883,11 +2883,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, for (int i = 0; i < vring_num; i++) { struct rte_vhost_vring vring; - char vhost_vring[18]; - - rte_vhost_get_vhost_vring(vid, i, &vring); - snprintf(vhost_vring, 16, "vring_%d_size", i); - smap_add_format(args, vhost_vring, "%d", vring.size); + smap_add_nocopy(args, + xasprintf("vring_%d_size", i), + xasprintf("%d", vring.size)); } ovs_mutex_unlock(&dev->mutex); What do you think? Thanks, Ben.
Ben Pfaff <blp@ovn.org> writes: > On Wed, Jun 13, 2018 at 03:43:04PM -0400, Aaron Conole wrote: >> lib/netdev-dpdk.c: In function : >> lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last format character [-Wformat-truncation=] >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> ^ >> lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a destination of size 16 >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Since vring_num is 16 bits, the largest value ever would only be 17 bytes, >> including the terminating nul. Stretch it to 18 bytes (as a precaution >> against a signed value, which again would never happen). >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> --- >> lib/netdev-dpdk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 2e2f568b8..e75943bb2 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, >> >> for (int i = 0; i < vring_num; i++) { >> struct rte_vhost_vring vring; >> - char vhost_vring[16]; >> + char vhost_vring[18]; >> >> rte_vhost_get_vhost_vring(vid, i, &vring); >> snprintf(vhost_vring, 16, "vring_%d_size", i); > > Thanks for the improvement. > > Instead of calculating at all, I think it would be less error-prone to > avoid it, something like this: > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index fd496592ba56..040b17f8a34a 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2883,11 +2883,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, > > for (int i = 0; i < vring_num; i++) { > struct rte_vhost_vring vring; > - char vhost_vring[18]; > - > - rte_vhost_get_vhost_vring(vid, i, &vring); > - snprintf(vhost_vring, 16, "vring_%d_size", i); > - smap_add_format(args, vhost_vring, "%d", vring.size); > + smap_add_nocopy(args, > + xasprintf("vring_%d_size", i), > + xasprintf("%d", vring.size)); > } > > ovs_mutex_unlock(&dev->mutex); > > What do you think? That looks *much* better. I completely missed that API. > Thanks, > > Ben.
Ben Pfaff <blp@ovn.org> writes: > On Wed, Jun 13, 2018 at 03:43:04PM -0400, Aaron Conole wrote: >> lib/netdev-dpdk.c: In function : >> lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last format character [-Wformat-truncation=] >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> ^ >> lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a destination of size 16 >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Since vring_num is 16 bits, the largest value ever would only be 17 bytes, >> including the terminating nul. Stretch it to 18 bytes (as a precaution >> against a signed value, which again would never happen). >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> --- >> lib/netdev-dpdk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 2e2f568b8..e75943bb2 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, >> >> for (int i = 0; i < vring_num; i++) { >> struct rte_vhost_vring vring; >> - char vhost_vring[16]; >> + char vhost_vring[18]; >> >> rte_vhost_get_vhost_vring(vid, i, &vring); >> snprintf(vhost_vring, 16, "vring_%d_size", i); > > Thanks for the improvement. > > Instead of calculating at all, I think it would be less error-prone to > avoid it, something like this: > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index fd496592ba56..040b17f8a34a 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2883,11 +2883,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, > > for (int i = 0; i < vring_num; i++) { > struct rte_vhost_vring vring; > - char vhost_vring[18]; > - > - rte_vhost_get_vhost_vring(vid, i, &vring); Just one thing... should you submit this (or merge it) - don't drop this line :-) > - snprintf(vhost_vring, 16, "vring_%d_size", i); > - smap_add_format(args, vhost_vring, "%d", vring.size); > + smap_add_nocopy(args, > + xasprintf("vring_%d_size", i), > + xasprintf("%d", vring.size)); > } > > ovs_mutex_unlock(&dev->mutex); > > What do you think? > > Thanks, > > Ben.
On Thu, Jun 14, 2018 at 05:11:39PM -0400, Aaron Conole wrote: > Ben Pfaff <blp@ovn.org> writes: > > > On Wed, Jun 13, 2018 at 03:43:04PM -0400, Aaron Conole wrote: > >> lib/netdev-dpdk.c: In function : > >> lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last format character [-Wformat-truncation=] > >> snprintf(vhost_vring, 16, "vring_%d_size", i); > >> ^ > >> lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a destination of size 16 > >> snprintf(vhost_vring, 16, "vring_%d_size", i); > >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> > >> Since vring_num is 16 bits, the largest value ever would only be 17 bytes, > >> including the terminating nul. Stretch it to 18 bytes (as a precaution > >> against a signed value, which again would never happen). > >> > >> Signed-off-by: Aaron Conole <aconole@redhat.com> > >> --- > >> lib/netdev-dpdk.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index 2e2f568b8..e75943bb2 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, > >> > >> for (int i = 0; i < vring_num; i++) { > >> struct rte_vhost_vring vring; > >> - char vhost_vring[16]; > >> + char vhost_vring[18]; > >> > >> rte_vhost_get_vhost_vring(vid, i, &vring); > >> snprintf(vhost_vring, 16, "vring_%d_size", i); > > > > Thanks for the improvement. > > > > Instead of calculating at all, I think it would be less error-prone to > > avoid it, something like this: > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index fd496592ba56..040b17f8a34a 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -2883,11 +2883,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, > > > > for (int i = 0; i < vring_num; i++) { > > struct rte_vhost_vring vring; > > - char vhost_vring[18]; > > - > > - rte_vhost_get_vhost_vring(vid, i, &vring); > > Just one thing... should you submit this (or merge it) - don't drop this > line :-) I was hoping that you would submit it, would you mind?
Ben Pfaff <blp@ovn.org> writes: > On Thu, Jun 14, 2018 at 05:11:39PM -0400, Aaron Conole wrote: >> Ben Pfaff <blp@ovn.org> writes: >> >> > On Wed, Jun 13, 2018 at 03:43:04PM -0400, Aaron Conole wrote: >> >> lib/netdev-dpdk.c: In function : >> >> lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last format character [-Wformat-truncation=] >> >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> >> ^ >> >> lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a destination of size 16 >> >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> >> >> Since vring_num is 16 bits, the largest value ever would only be 17 bytes, >> >> including the terminating nul. Stretch it to 18 bytes (as a precaution >> >> against a signed value, which again would never happen). >> >> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> >> --- >> >> lib/netdev-dpdk.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> >> index 2e2f568b8..e75943bb2 100644 >> >> --- a/lib/netdev-dpdk.c >> >> +++ b/lib/netdev-dpdk.c >> >> @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, >> >> >> >> for (int i = 0; i < vring_num; i++) { >> >> struct rte_vhost_vring vring; >> >> - char vhost_vring[16]; >> >> + char vhost_vring[18]; >> >> >> >> rte_vhost_get_vhost_vring(vid, i, &vring); >> >> snprintf(vhost_vring, 16, "vring_%d_size", i); >> > >> > Thanks for the improvement. >> > >> > Instead of calculating at all, I think it would be less error-prone to >> > avoid it, something like this: >> > >> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> > index fd496592ba56..040b17f8a34a 100644 >> > --- a/lib/netdev-dpdk.c >> > +++ b/lib/netdev-dpdk.c >> > @@ -2883,11 +2883,9 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, >> > >> > for (int i = 0; i < vring_num; i++) { >> > struct rte_vhost_vring vring; >> > - char vhost_vring[18]; >> > - >> > - rte_vhost_get_vhost_vring(vid, i, &vring); >> >> Just one thing... should you submit this (or merge it) - don't drop this >> line :-) > > I was hoping that you would submit it, would you mind? Sure thing. I'll send it.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 2e2f568b8..e75943bb2 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2859,7 +2859,7 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, for (int i = 0; i < vring_num; i++) { struct rte_vhost_vring vring; - char vhost_vring[16]; + char vhost_vring[18]; rte_vhost_get_vhost_vring(vid, i, &vring); snprintf(vhost_vring, 16, "vring_%d_size", i);
lib/netdev-dpdk.c: In function : lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last format character [-Wformat-truncation=] snprintf(vhost_vring, 16, "vring_%d_size", i); ^ lib/netdev-dpdk.c:2865:9: note: output between 13 and 17 bytes into a destination of size 16 snprintf(vhost_vring, 16, "vring_%d_size", i); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Since vring_num is 16 bits, the largest value ever would only be 17 bytes, including the terminating nul. Stretch it to 18 bytes (as a precaution against a signed value, which again would never happen). Signed-off-by: Aaron Conole <aconole@redhat.com> --- lib/netdev-dpdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)