2018-09-08 10:40:32

by zhong jiang

[permalink] [raw]
Subject: [PATCH 0/2] s390: Use ARRAY_SIZE instead of reimplementing its function

I find the issue with the help of Coccinelle.

zhong jiang (2):
s390: vmlogrdr: Use ARRAY_SIZE instead of reimplementing its function
s390: qeth_core_mpc: Use ARRAY_SIZE instead of reimplementing its
function

drivers/s390/char/vmlogrdr.c | 2 +-
drivers/s390/net/qeth_core_mpc.c | 7 ++-----
2 files changed, 3 insertions(+), 6 deletions(-)

--
1.7.12.4



2018-09-08 10:40:03

by zhong jiang

[permalink] [raw]
Subject: [PATCH 1/2] s390: vmlogrdr: Use ARRAY_SIZE instead of reimplementing its function

ARRAY_SIZE has implemented its function. we prefer to use the function
rather than the open code.

Signed-off-by: zhong jiang <[email protected]>
---
drivers/s390/char/vmlogrdr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/char/vmlogrdr.c b/drivers/s390/char/vmlogrdr.c
index 069b9ef..58333cb 100644
--- a/drivers/s390/char/vmlogrdr.c
+++ b/drivers/s390/char/vmlogrdr.c
@@ -153,7 +153,7 @@ static void vmlogrdr_iucv_message_pending(struct iucv_path *,
}
};

-#define MAXMINOR (sizeof(sys_ser)/sizeof(struct vmlogrdr_priv_t))
+#define MAXMINOR ARRAY_SIZE(sys_ser)

static char FENCE[] = {"EOR"};
static int vmlogrdr_major = 0;
--
1.7.12.4


2018-09-08 10:40:52

by zhong jiang

[permalink] [raw]
Subject: [PATCH 2/2] s390: qeth_core_mpc: Use ARRAY_SIZE instead of reimplementing its function

ARRAY_SIZE has implemented its function. we prefer to use the function
rather than the open code.

Signed-off-by: zhong jiang <[email protected]>
---
drivers/s390/net/qeth_core_mpc.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/net/qeth_core_mpc.c b/drivers/s390/net/qeth_core_mpc.c
index 5bcb8da..e8263de 100644
--- a/drivers/s390/net/qeth_core_mpc.c
+++ b/drivers/s390/net/qeth_core_mpc.c
@@ -222,8 +222,7 @@ struct ipa_rc_msg {
char *qeth_get_ipa_msg(enum qeth_ipa_return_codes rc)
{
int x = 0;
- qeth_ipa_rc_msg[sizeof(qeth_ipa_rc_msg) /
- sizeof(struct ipa_rc_msg) - 1].rc = rc;
+ qeth_ipa_rc_msg[ARRAY_SIZE(qeth_ipa_rc_msg) - 1].rc = rc;
while (qeth_ipa_rc_msg[x].rc != rc)
x++;
return qeth_ipa_rc_msg[x].msg;
@@ -270,9 +269,7 @@ struct ipa_cmd_names {
char *qeth_get_ipa_cmd_name(enum qeth_ipa_cmds cmd)
{
int x = 0;
- qeth_ipa_cmd_names[
- sizeof(qeth_ipa_cmd_names) /
- sizeof(struct ipa_cmd_names)-1].cmd = cmd;
+ qeth_ipa_cmd_names[ARRAY_SIZE(qeth_ipa_cmd_names) - 1].cmd = cmd;
while (qeth_ipa_cmd_names[x].cmd != cmd)
x++;
return qeth_ipa_cmd_names[x].name;
--
1.7.12.4


2018-09-08 13:02:45

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH 1/2] s390: vmlogrdr: Use ARRAY_SIZE instead of reimplementing its function

On Sat, 8 Sep 2018 18:26:27 +0800, zhong jiang wrote:
> ARRAY_SIZE has implemented its function. we prefer to use the function
> rather than the open code.
>
> Signed-off-by: zhong jiang <[email protected]>
> ---
> drivers/s390/char/vmlogrdr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/s390/char/vmlogrdr.c b/drivers/s390/char/vmlogrdr.c
> index 069b9ef..58333cb 100644
> --- a/drivers/s390/char/vmlogrdr.c
> +++ b/drivers/s390/char/vmlogrdr.c
> @@ -153,7 +153,7 @@ static void vmlogrdr_iucv_message_pending(struct iucv_path *,
> }
> };
>
> -#define MAXMINOR (sizeof(sys_ser)/sizeof(struct vmlogrdr_priv_t))
> +#define MAXMINOR ARRAY_SIZE(sys_ser)
>
> static char FENCE[] = {"EOR"};
> static int vmlogrdr_major = 0;

Reviewed-by: Jean Delvare <[email protected]>

--
Jean Delvare
SUSE L3 Support

2018-09-08 13:12:53

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH 2/2] s390: qeth_core_mpc: Use ARRAY_SIZE instead of reimplementing its function

On Sat, 8 Sep 2018 18:26:28 +0800, zhong jiang wrote:
> ARRAY_SIZE has implemented its function. we prefer to use the function
> rather than the open code.
>
> Signed-off-by: zhong jiang <[email protected]>
> ---
> drivers/s390/net/qeth_core_mpc.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/s390/net/qeth_core_mpc.c b/drivers/s390/net/qeth_core_mpc.c
> index 5bcb8da..e8263de 100644
> --- a/drivers/s390/net/qeth_core_mpc.c
> +++ b/drivers/s390/net/qeth_core_mpc.c
> @@ -222,8 +222,7 @@ struct ipa_rc_msg {
> char *qeth_get_ipa_msg(enum qeth_ipa_return_codes rc)
> {
> int x = 0;
> - qeth_ipa_rc_msg[sizeof(qeth_ipa_rc_msg) /
> - sizeof(struct ipa_rc_msg) - 1].rc = rc;
> + qeth_ipa_rc_msg[ARRAY_SIZE(qeth_ipa_rc_msg) - 1].rc = rc;
> while (qeth_ipa_rc_msg[x].rc != rc)
> x++;
> return qeth_ipa_rc_msg[x].msg;
> @@ -270,9 +269,7 @@ struct ipa_cmd_names {
> char *qeth_get_ipa_cmd_name(enum qeth_ipa_cmds cmd)
> {
> int x = 0;
> - qeth_ipa_cmd_names[
> - sizeof(qeth_ipa_cmd_names) /
> - sizeof(struct ipa_cmd_names)-1].cmd = cmd;
> + qeth_ipa_cmd_names[ARRAY_SIZE(qeth_ipa_cmd_names) - 1].cmd = cmd;
> while (qeth_ipa_cmd_names[x].cmd != cmd)
> x++;
> return qeth_ipa_cmd_names[x].name;

Reviewed-by: Jean Delvare <[email protected]>

BTW, this code looks racy. It is modifying a global array member
without any locking. If there is any chance that two instances of
qeth_check_ipa_data() are running in parallel and both have unknown
command (or unknown rc), we could end up overrunning either
qeth_ipa_rc_msg[] or qeth_ipa_cmd_names[]. OK, that's unlikely to
happen in practice (I suppose unknown command and unknown rc are not
supposed to happen in the first place), but that's still bad programming
style. I'll try to come up with something better...

--
Jean Delvare
SUSE L3 Support

2018-09-10 06:39:01

by Martin Schwidefsky

[permalink] [raw]
Subject: Re: [PATCH 0/2] s390: Use ARRAY_SIZE instead of reimplementing its function

On Sat, 8 Sep 2018 18:26:26 +0800
zhong jiang <[email protected]> wrote:

> I find the issue with the help of Coccinelle.
>
> zhong jiang (2):
> s390: vmlogrdr: Use ARRAY_SIZE instead of reimplementing its function
> s390: qeth_core_mpc: Use ARRAY_SIZE instead of reimplementing its
> function
>
> drivers/s390/char/vmlogrdr.c | 2 +-
> drivers/s390/net/qeth_core_mpc.c | 7 ++-----
> 2 files changed, 3 insertions(+), 6 deletions(-)

Both patches applied to linux-s390:features. Thanks.

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.