| Message ID | 20250106141731.1341034-1-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dm vdo: Fix W=1 allmodconfig build error | expand |
Hi John, I'm all for getting rid of warnings where we can, but I'm not convinced this is the right approach here. First, we use snprintf here because we really do want to limit the string to 15 bytes. Making this constant larger doesn't maintain that. This change also increases the size of struct vdo (in vdo.h) by about 50 bytes. While that's not a lot, it does seem unnecessary. In practice, we know that the our module name is only about 6 bytes, and the instance number will never be more than 3, so snprintf will never actually truncate anything here. Would checking for a non-zero return value suppress the error? I'm also confused because I don't understand the difference between this line and the similar use of snprintf at drivers/md/dm-vdo/funnel-workqueue.c:430, which I presume does not trigger a warning? Anyway, I'd be happy with a version that suppresses the warning without changing the size of any structures. Matt Sakai On 1/6/25 9:17 AM, John Garry wrote: > For an allmodconfig build, I currently get this error when building with > W=1: > > $ make W=1 drivers/md/dm-vdo/vdo.o > CALL scripts/checksyscalls.sh > DESCEND objtool > INSTALL libsubcmd_headers > CC [M] drivers/md/dm-vdo/vdo.o > drivers/md/dm-vdo/vdo.c: In function ‘vdo_make’: > drivers/md/dm-vdo/vdo.c:564:5: error: ‘%s’ directive output may be truncated writing up to 55 bytes into a region of size 16 [-Werror=format-truncation=] > "%s%u", MODULE_NAME, instance); > ^~ > drivers/md/dm-vdo/vdo.c:563:2: note: ‘snprintf’ output between 2 and 66 bytes into a destination of size 16 > snprintf(vdo->thread_name_prefix, sizeof(vdo->thread_name_prefix), > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > "%s%u", MODULE_NAME, instance); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Fix by defining MAX_VDO_WORK_QUEUE_NAME_LEN to be sum of MODULE_NAME_LEN > plus 11 - 11 comes from number of characters required to print max unsigned > int plus NUL terminator. > > Also relocate the include of sched.h to the only c file which it is > actually needed. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > diff --git a/drivers/md/dm-vdo/funnel-workqueue.c b/drivers/md/dm-vdo/funnel-workqueue.c > index ae11941c90a9..7e42342263ab 100644 > --- a/drivers/md/dm-vdo/funnel-workqueue.c > +++ b/drivers/md/dm-vdo/funnel-workqueue.c > @@ -11,6 +11,7 @@ > #include <linux/err.h> > #include <linux/kthread.h> > #include <linux/percpu.h> > +#include <linux/sched.h> > > #include "funnel-queue.h" > #include "logger.h" > diff --git a/drivers/md/dm-vdo/funnel-workqueue.h b/drivers/md/dm-vdo/funnel-workqueue.h > index b5be6e9e83bc..3447aac5b188 100644 > --- a/drivers/md/dm-vdo/funnel-workqueue.h > +++ b/drivers/md/dm-vdo/funnel-workqueue.h > @@ -6,12 +6,10 @@ > #ifndef VDO_WORK_QUEUE_H > #define VDO_WORK_QUEUE_H > > -#include <linux/sched.h> /* for TASK_COMM_LEN */ > - > #include "types.h" > > enum { > - MAX_VDO_WORK_QUEUE_NAME_LEN = TASK_COMM_LEN, > + MAX_VDO_WORK_QUEUE_NAME_LEN = MODULE_NAME_LEN + 11, > }; > > struct vdo_work_queue_type {
On 07/01/2025 01:24, Matthew Sakai wrote: > Hi John, Hi Matt, > > I'm all for getting rid of warnings where we can, but I'm not convinced > this is the right approach here. > > First, we use snprintf here because we really do want to limit the > string to 15 bytes. Out of curiosity, why is that? BTW, as I understand, for CONFIG_DM_VDO=m, the module name is "dm_vdo". So why different MODULE_NAME if MODULE is not defined ("dm-vdo")? > Making this constant larger doesn't maintain that. > This change also increases the size of struct vdo (in vdo.h) by about 50 > bytes. While that's not a lot, it does seem unnecessary. > > In practice, we know that the our module name is only about 6 bytes, and > the instance number will never be more than 3, so snprintf will never > actually truncate anything here. Would checking for a non-zero return > value suppress the error? > > I'm also confused because I don't understand the difference between this > line and the similar use of snprintf at drivers/md/dm-vdo/funnel- > workqueue.c:430, which I presume does not trigger a warning? For that snprintf usage, the compiler just doesn't know the possible length of @name, as it is a pointer. That is why it is a good reason to use snprintf. > > Anyway, I'd be happy with a version that suppresses the warning without > changing the size of any structures. > > Matt Sakai > Thanks, John
diff --git a/drivers/md/dm-vdo/funnel-workqueue.c b/drivers/md/dm-vdo/funnel-workqueue.c index ae11941c90a9..7e42342263ab 100644 --- a/drivers/md/dm-vdo/funnel-workqueue.c +++ b/drivers/md/dm-vdo/funnel-workqueue.c @@ -11,6 +11,7 @@ #include <linux/err.h> #include <linux/kthread.h> #include <linux/percpu.h> +#include <linux/sched.h> #include "funnel-queue.h" #include "logger.h" diff --git a/drivers/md/dm-vdo/funnel-workqueue.h b/drivers/md/dm-vdo/funnel-workqueue.h index b5be6e9e83bc..3447aac5b188 100644 --- a/drivers/md/dm-vdo/funnel-workqueue.h +++ b/drivers/md/dm-vdo/funnel-workqueue.h @@ -6,12 +6,10 @@ #ifndef VDO_WORK_QUEUE_H #define VDO_WORK_QUEUE_H -#include <linux/sched.h> /* for TASK_COMM_LEN */ - #include "types.h" enum { - MAX_VDO_WORK_QUEUE_NAME_LEN = TASK_COMM_LEN, + MAX_VDO_WORK_QUEUE_NAME_LEN = MODULE_NAME_LEN + 11, }; struct vdo_work_queue_type {
For an allmodconfig build, I currently get this error when building with W=1: $ make W=1 drivers/md/dm-vdo/vdo.o CALL scripts/checksyscalls.sh DESCEND objtool INSTALL libsubcmd_headers CC [M] drivers/md/dm-vdo/vdo.o drivers/md/dm-vdo/vdo.c: In function ‘vdo_make’: drivers/md/dm-vdo/vdo.c:564:5: error: ‘%s’ directive output may be truncated writing up to 55 bytes into a region of size 16 [-Werror=format-truncation=] "%s%u", MODULE_NAME, instance); ^~ drivers/md/dm-vdo/vdo.c:563:2: note: ‘snprintf’ output between 2 and 66 bytes into a destination of size 16 snprintf(vdo->thread_name_prefix, sizeof(vdo->thread_name_prefix), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "%s%u", MODULE_NAME, instance); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Fix by defining MAX_VDO_WORK_QUEUE_NAME_LEN to be sum of MODULE_NAME_LEN plus 11 - 11 comes from number of characters required to print max unsigned int plus NUL terminator. Also relocate the include of sched.h to the only c file which it is actually needed. Signed-off-by: John Garry <john.g.garry@oracle.com>