[ovs-dev,2/2] netdev-dpdk: fix snprintf call
diff mbox series

Message ID 20180613194304.31548-2-aconole@redhat.com
State Changes Requested
Headers show
Series
  • [ovs-dev,1/2] lldp: fix string warnings
Related show

Commit Message

Aaron Conole June 13, 2018, 7:43 p.m. UTC
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(-)

Comments

Yifeng Sun June 14, 2018, 5:29 p.m. UTC | #1
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
>
Ben Pfaff June 14, 2018, 8:28 p.m. UTC | #2
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.
Aaron Conole June 14, 2018, 9:04 p.m. UTC | #3
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.
Aaron Conole June 14, 2018, 9:11 p.m. UTC | #4
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.
Ben Pfaff June 14, 2018, 9:32 p.m. UTC | #5
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?
Aaron Conole June 14, 2018, 10:05 p.m. UTC | #6
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.

Patch
diff mbox series

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);