| Message ID | 20231023233704.1185154-4-asmadeus@codewreck.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Small patches for 6.7 | expand |
On Tuesday, October 24, 2023 1:37:04 AM CEST Dominique Martinet wrote: > Use a local variable to make the compiler happy about this warning: > net/9p/trans_xen.c: In function ‘xen_9pfs_front_changed’: > net/9p/trans_xen.c:444:39: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 8 [-Wformat-overflow=] > 444 | sprintf(str, "ring-ref%d", i); > | ^~ > In function ‘xen_9pfs_front_init’, > inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:516:8, > inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:504:13: > net/9p/trans_xen.c:444:30: note: directive argument in the range [-2147483644, 2147483646] > 444 | sprintf(str, "ring-ref%d", i); > | ^~~~~~~~~~~~ > net/9p/trans_xen.c:444:17: note: ‘sprintf’ output between 10 and 20 bytes into a destination of size 16 > 444 | sprintf(str, "ring-ref%d", i); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > net/9p/trans_xen.c: In function ‘xen_9pfs_front_changed’: > net/9p/trans_xen.c:450:45: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 2 [-Wformat-overflow=] > 450 | sprintf(str, "event-channel-%d", i); > | ^~ > In function ‘xen_9pfs_front_init’, > inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:516:8, > inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:504:13: > net/9p/trans_xen.c:450:30: note: directive argument in the range [-2147483644, 2147483646] > 450 | sprintf(str, "event-channel-%d", i); > | ^~~~~~~~~~~~~~~~~~ > net/9p/trans_xen.c:450:17: note: ‘sprintf’ output between 16 and 26 bytes into a destination of size 16 > 450 | sprintf(str, "event-channel-%d", i); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > There is no change in logic: there only are a constant number of rings, > and there also already is a BUILD_BUG_ON that checks if that constant > goes over 9 as anything bigger would no longer fit the event-channel-%d > destination size. > > In theory having that size as part of the struct means it could be > modified by another thread and makes the compiler lose track of possible > values for 'i' here, using a local variable makes it happy. Or ... what about dropping struct's 'num_rings' member and using the constant XEN_9PFS_NUM_RINGS everywhere instead? As this is really a compile-time value after all. > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> > --- > While looking at warnings with W=1, I noticed this one in net/9p. > > This is silly but shouldn't hurt, num_rings is never changed so there is > no risk of introducing a race here, it's just helping the compiler a > bit. > > net/9p is also now warning-free at W=1 > > net/9p/trans_xen.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c > index 1fffe2bed5b0..79e91f31a84a 100644 > --- a/net/9p/trans_xen.c > +++ b/net/9p/trans_xen.c > @@ -382,7 +382,7 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) > struct xenbus_transaction xbt; > struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev); > char *versions, *v; > - unsigned int max_rings, max_ring_order, len = 0; > + unsigned int num_rings, max_rings, max_ring_order, len = 0; > > versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len); > if (IS_ERR(versions)) > @@ -408,15 +408,15 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) > if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order)) > p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2; > > - priv->num_rings = XEN_9PFS_NUM_RINGS; > - priv->rings = kcalloc(priv->num_rings, sizeof(*priv->rings), > + num_rings = priv->num_rings = XEN_9PFS_NUM_RINGS; > + priv->rings = kcalloc(num_rings, sizeof(*priv->rings), > GFP_KERNEL); > if (!priv->rings) { > kfree(priv); > return -ENOMEM; > } > > - for (i = 0; i < priv->num_rings; i++) { > + for (i = 0; i < num_rings; i++) { > priv->rings[i].priv = priv; > ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i], > max_ring_order); > @@ -434,10 +434,11 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) > if (ret) > goto error_xenbus; > ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", > - priv->num_rings); > + num_rings); > if (ret) > goto error_xenbus; > - for (i = 0; i < priv->num_rings; i++) { > + > + for (i = 0; i < num_rings; i++) { > char str[16]; > > BUILD_BUG_ON(XEN_9PFS_NUM_RINGS > 9); >
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index 1fffe2bed5b0..79e91f31a84a 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -382,7 +382,7 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) struct xenbus_transaction xbt; struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev); char *versions, *v; - unsigned int max_rings, max_ring_order, len = 0; + unsigned int num_rings, max_rings, max_ring_order, len = 0; versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len); if (IS_ERR(versions)) @@ -408,15 +408,15 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order)) p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2; - priv->num_rings = XEN_9PFS_NUM_RINGS; - priv->rings = kcalloc(priv->num_rings, sizeof(*priv->rings), + num_rings = priv->num_rings = XEN_9PFS_NUM_RINGS; + priv->rings = kcalloc(num_rings, sizeof(*priv->rings), GFP_KERNEL); if (!priv->rings) { kfree(priv); return -ENOMEM; } - for (i = 0; i < priv->num_rings; i++) { + for (i = 0; i < num_rings; i++) { priv->rings[i].priv = priv; ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i], max_ring_order); @@ -434,10 +434,11 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) if (ret) goto error_xenbus; ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", - priv->num_rings); + num_rings); if (ret) goto error_xenbus; - for (i = 0; i < priv->num_rings; i++) { + + for (i = 0; i < num_rings; i++) { char str[16]; BUILD_BUG_ON(XEN_9PFS_NUM_RINGS > 9);
Use a local variable to make the compiler happy about this warning: net/9p/trans_xen.c: In function ‘xen_9pfs_front_changed’: net/9p/trans_xen.c:444:39: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 8 [-Wformat-overflow=] 444 | sprintf(str, "ring-ref%d", i); | ^~ In function ‘xen_9pfs_front_init’, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:516:8, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:504:13: net/9p/trans_xen.c:444:30: note: directive argument in the range [-2147483644, 2147483646] 444 | sprintf(str, "ring-ref%d", i); | ^~~~~~~~~~~~ net/9p/trans_xen.c:444:17: note: ‘sprintf’ output between 10 and 20 bytes into a destination of size 16 444 | sprintf(str, "ring-ref%d", i); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/9p/trans_xen.c: In function ‘xen_9pfs_front_changed’: net/9p/trans_xen.c:450:45: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 2 [-Wformat-overflow=] 450 | sprintf(str, "event-channel-%d", i); | ^~ In function ‘xen_9pfs_front_init’, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:516:8, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:504:13: net/9p/trans_xen.c:450:30: note: directive argument in the range [-2147483644, 2147483646] 450 | sprintf(str, "event-channel-%d", i); | ^~~~~~~~~~~~~~~~~~ net/9p/trans_xen.c:450:17: note: ‘sprintf’ output between 16 and 26 bytes into a destination of size 16 450 | sprintf(str, "event-channel-%d", i); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There is no change in logic: there only are a constant number of rings, and there also already is a BUILD_BUG_ON that checks if that constant goes over 9 as anything bigger would no longer fit the event-channel-%d destination size. In theory having that size as part of the struct means it could be modified by another thread and makes the compiler lose track of possible values for 'i' here, using a local variable makes it happy. Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- While looking at warnings with W=1, I noticed this one in net/9p. This is silly but shouldn't hurt, num_rings is never changed so there is no risk of introducing a race here, it's just helping the compiler a bit. net/9p is also now warning-free at W=1 net/9p/trans_xen.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)