diff mbox series

net/mana: avoid the use of variable length array

Message ID 1741135052-2039-1-git-send-email-longli@linuxonhyperv.com (mailing list archive)
State Changes Requested, archived
Delegated to: Stephen Hemminger
Headers
Series net/mana: avoid the use of variable length array |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS

Commit Message

Long Li March 5, 2025, 12:37 a.m. UTC 
From: Long Li <longli@microsoft.com>

The pathname can be defined as name[MAX_PATH]. This makes the driver
compilable using MSVC.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/mana/mana.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Morten Brørup March 5, 2025, 9 a.m. UTC | #1 
> From: longli@linuxonhyperv.com [mailto:longli@linuxonhyperv.com]
> Sent: Wednesday, 5 March 2025 01.38
> 
> From: Long Li <longli@microsoft.com>
> 
> The pathname can be defined as name[MAX_PATH]. This makes the driver
> compilable using MSVC.

The name is PATH_MAX, not MAX_PATH. It's correct in the code.

> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/net/mana/mana.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
> index c37c4e3444..d12dff6ce1 100644
> --- a/drivers/net/mana/mana.c
> +++ b/drivers/net/mana/mana.c
> @@ -36,11 +36,8 @@ static rte_spinlock_t mana_shared_data_lock =
> RTE_SPINLOCK_INITIALIZER;
> 
>  /* Allocate a buffer on the stack and fill it with a printf format
> string. */
>  #define MANA_MKSTR(name, ...) \

The macro is no longer generic, but tied to a path name, so the macro's name and description should be updated accordingly. E.g.:

/* Allocate a path name buffer on the stack and fill it with a printf format string. */
#define MANA_MKPATHSTR(name, ...) \

Don't forget search-replace for uses of the macro. (It's being used 6 times in this file.)

> -	int mkstr_size_##name = snprintf(NULL, 0, "" __VA_ARGS__); \
> -	char name[mkstr_size_##name + 1]; \
> -	\
> -	memset(name, 0, mkstr_size_##name + 1); \
> -	snprintf(name, sizeof(name), "" __VA_ARGS__)
> +	char name[PATH_MAX]; \
> +	snprintf(name, PATH_MAX, "" __VA_ARGS__)
> 
>  int mana_logtype_driver;
>  int mana_logtype_init;
> --
> 2.34.1

With suggested changes,
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Andre Muezerie March 6, 2025, 2:17 a.m. UTC | #2 
On Tue, Mar 04, 2025 at 04:37:32PM -0800, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
> 
> The pathname can be defined as name[MAX_PATH]. This makes the driver
> compilable using MSVC.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/net/mana/mana.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
> index c37c4e3444..d12dff6ce1 100644
> --- a/drivers/net/mana/mana.c
> +++ b/drivers/net/mana/mana.c
> @@ -36,11 +36,8 @@ static rte_spinlock_t mana_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
>  
>  /* Allocate a buffer on the stack and fill it with a printf format string. */
>  #define MANA_MKSTR(name, ...) \
> -	int mkstr_size_##name = snprintf(NULL, 0, "" __VA_ARGS__); \
> -	char name[mkstr_size_##name + 1]; \
> -	\
> -	memset(name, 0, mkstr_size_##name + 1); \
> -	snprintf(name, sizeof(name), "" __VA_ARGS__)
> +	char name[PATH_MAX]; \
> +	snprintf(name, PATH_MAX, "" __VA_ARGS__)
>  
>  int mana_logtype_driver;
>  int mana_logtype_init;
> -- 
> 2.34.1

Did you try to remove the line below from mana/meson.build?
That line prevents the compiler from complain about VLAs. If
the driver is VLA-free after this fix it would be great
if the compiler was allowed to complain about VLAs (default
in DPDK project).

If the code still compiles without this line then it should be
safe to remove it:

cflags += no_wvla_cflag
--
Andre Muezerie
Long Li March 6, 2025, 10:01 p.m. UTC | #3 
> Subject: Re: [PATCH] net/mana: avoid the use of variable length array
> 
> On Tue, Mar 04, 2025 at 04:37:32PM -0800, longli@linuxonhyperv.com wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > The pathname can be defined as name[MAX_PATH]. This makes the driver
> > compilable using MSVC.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  drivers/net/mana/mana.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c index
> > c37c4e3444..d12dff6ce1 100644
> > --- a/drivers/net/mana/mana.c
> > +++ b/drivers/net/mana/mana.c
> > @@ -36,11 +36,8 @@ static rte_spinlock_t mana_shared_data_lock =
> > RTE_SPINLOCK_INITIALIZER;
> >
> >  /* Allocate a buffer on the stack and fill it with a printf format
> > string. */  #define MANA_MKSTR(name, ...) \
> > -	int mkstr_size_##name = snprintf(NULL, 0, "" __VA_ARGS__); \
> > -	char name[mkstr_size_##name + 1]; \
> > -	\
> > -	memset(name, 0, mkstr_size_##name + 1); \
> > -	snprintf(name, sizeof(name), "" __VA_ARGS__)
> > +	char name[PATH_MAX]; \
> > +	snprintf(name, PATH_MAX, "" __VA_ARGS__)
> >
> >  int mana_logtype_driver;
> >  int mana_logtype_init;
> > --
> > 2.34.1
> 
> Did you try to remove the line below from mana/meson.build?
> That line prevents the compiler from complain about VLAs. If the driver is VLA-
> free after this fix it would be great if the compiler was allowed to complain about
> VLAs (default in DPDK project).
> 
> If the code still compiles without this line then it should be safe to remove it:
> 
> cflags += no_wvla_cflag
> --
> Andre Muezerie

I tried, it gives warnings on other two places using VLA:

[1688/3164] Compiling C object drivers/libtmp_rte_net_mana.a.p/net_mana_mr.c.o
../drivers/net/mana/mr.c: In function 'mana_new_pmd_mr':
../drivers/net/mana/mr.c:41:16: warning: ISO C90 forbids variable length array 'ranges' [-Wvla]
   41 |         struct mana_range ranges[pool->nb_mem_chunks];
      |                ^~~~~~~~~~
[1710/3164] Compiling C object drivers/libtmp_rte_net_mana.a.p/net_mana_rx.c.o
../drivers/net/mana/rx.c: In function 'mana_start_rx_queues':
../drivers/net/mana/rx.c:244:16: warning: ISO C90 forbids variable length array 'ind_tbl' [-Wvla]
  244 |         struct ibv_wq *ind_tbl[priv->num_queues];
      |                ^~~~~~

I'll fix them up and send a patch.
diff mbox series

Patch

diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
index c37c4e3444..d12dff6ce1 100644
--- a/drivers/net/mana/mana.c
+++ b/drivers/net/mana/mana.c
@@ -36,11 +36,8 @@  static rte_spinlock_t mana_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
 
 /* Allocate a buffer on the stack and fill it with a printf format string. */
 #define MANA_MKSTR(name, ...) \
-	int mkstr_size_##name = snprintf(NULL, 0, "" __VA_ARGS__); \
-	char name[mkstr_size_##name + 1]; \
-	\
-	memset(name, 0, mkstr_size_##name + 1); \
-	snprintf(name, sizeof(name), "" __VA_ARGS__)
+	char name[PATH_MAX]; \
+	snprintf(name, PATH_MAX, "" __VA_ARGS__)
 
 int mana_logtype_driver;
 int mana_logtype_init;