All of lore.kernel.org
 help / color / mirror / Atom feed
* [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings
@ 2021-01-11 11:30 mrezanin
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 1/3] Fix net.c warning on GCC 11 mrezanin
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: mrezanin @ 2021-01-11 11:30 UTC (permalink / raw)
  To: qemu-devel

From: Miroslav Rezanina <mrezanin@redhat.com>

Compiling qemu using GCC 11 we got several new warnings. To allow
build with --enable-werror, we need to solve issues generating these
warnings.

Signed-of-by: Miroslav Rezanina <mrezanin@redhat.com>

Miroslav Rezanina (3):
  Fix net.c warning on GCC 11
  s390x: Fix vm name copy length
  Fix tcg_out_op argument mismatch warning

 net/eth.c                    | 3 +++
 target/s390x/kvm.c           | 2 +-
 target/s390x/misc_helper.c   | 4 +++-
 tcg/aarch64/tcg-target.c.inc | 3 +--
 tcg/sparc/tcg-target.c.inc   | 3 +--
 5 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.18.4



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RHEL7 qemu-kvm PATCH 1/3] Fix net.c warning on GCC 11
  2021-01-11 11:30 [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings mrezanin
@ 2021-01-11 11:30 ` mrezanin
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length mrezanin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: mrezanin @ 2021-01-11 11:30 UTC (permalink / raw)
  To: qemu-devel

From: Miroslav Rezanina <mrezanin@redhat.com>

When building qemu with GCC 11, compiling eth.c file produce following warning:

   warning: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Warray-bounds]

This caused by retyping from ip6_ext_hdr to ip6_ext_hdr_routing that has more
attributes.

As this usage is expected, suppress the warning temporarily through the function
using this retyping.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
 net/eth.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/eth.c b/net/eth.c
index 1e0821c5f8..b9bdd0435c 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -405,6 +405,8 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
                         struct ip6_ext_hdr *ext_hdr,
                         struct in6_address *dst_addr)
 {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
     struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
 
     if ((rthdr->rtype == 2) &&
@@ -426,6 +428,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
     }
 
     return false;
+#pragma GCC diagnostic pop
 }
 
 static bool
-- 
2.18.4



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 11:30 [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings mrezanin
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 1/3] Fix net.c warning on GCC 11 mrezanin
@ 2021-01-11 11:30 ` mrezanin
  2021-01-11 12:10   ` Philippe Mathieu-Daudé
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning mrezanin
  2021-01-11 11:39 ` [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings no-reply
  3 siblings, 1 reply; 17+ messages in thread
From: mrezanin @ 2021-01-11 11:30 UTC (permalink / raw)
  To: qemu-devel

From: Miroslav Rezanina <mrezanin@redhat.com>

There are two cases when vm name is copied but closing \0 can be lost
in case name is too long (>=256 characters).

Updating length to copy so there is space for closing \0.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
 target/s390x/kvm.c         | 2 +-
 target/s390x/misc_helper.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index b8385e6b95..2313b5727e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
      */
     if (qemu_name) {
         strncpy((char *)sysib.ext_names[0], qemu_name,
-                sizeof(sysib.ext_names[0]));
+                sizeof(sysib.ext_names[0]) - 1);
     } else {
         strcpy((char *)sysib.ext_names[0], "KVMguest");
     }
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 58dbc023eb..7c478b9e58 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, uint64_t r0, uint64_t r1)
                 ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name,
                            MIN(sizeof(sysib.sysib_322.vm[0].name),
                                strlen(qemu_name)));
+		memset((char *)sysib.sysib_322.ext_names[0], 0, 
+		       sizeof(sysib.sysib_322.ext_names[0]));
                 strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name,
-                        sizeof(sysib.sysib_322.ext_names[0]));
+                        sizeof(sysib.sysib_322.ext_names[0]) - 1);
             } else {
                 ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8);
                 strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest");
-- 
2.18.4



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning
  2021-01-11 11:30 [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings mrezanin
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 1/3] Fix net.c warning on GCC 11 mrezanin
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length mrezanin
@ 2021-01-11 11:30 ` mrezanin
  2021-01-11 12:15   ` Philippe Mathieu-Daudé
  2021-01-11 11:39 ` [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings no-reply
  3 siblings, 1 reply; 17+ messages in thread
From: mrezanin @ 2021-01-11 11:30 UTC (permalink / raw)
  To: qemu-devel

From: Miroslav Rezanina <mrezanin@redhat.com>

There's prototype mismatch between tcg/tcg.c and tcg/aarch/tcg-target.c.inc:

tcg.c:

    static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
                           const int *const_args);

tcg-target.c.inc:

    static void tcg_out_op(TCGContext *s, TCGOpcode opc,
                           const TCGArg args[TCG_MAX_OP_ARGS],
                           const int const_args[TCG_MAX_OP_ARGS])

This missmatch cause warnings on GCC 11:

    tcg/aarch64/tcg-target.c.inc:1855:37: error: argument 3 of type 'const TCGArg[16]' {aka 'const long unsigned int[16]'} with mismatched bound [-Werror=array-parameter=]
    tcg/aarch64/tcg-target.c.inc:1856:34: error: argument 4 of type 'const int[16]' with mismatched bound [-Werror=array-parameter=]

Only architectures with this definition are aarch and sparc. Fixing both archs to use
proper argument type.
---
 tcg/aarch64/tcg-target.c.inc | 3 +--
 tcg/sparc/tcg-target.c.inc   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
index 26f71cb599..fe6bdbf721 100644
--- a/tcg/aarch64/tcg-target.c.inc
+++ b/tcg/aarch64/tcg-target.c.inc
@@ -1852,8 +1852,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
 static tcg_insn_unit *tb_ret_addr;
 
 static void tcg_out_op(TCGContext *s, TCGOpcode opc,
-                       const TCGArg args[TCG_MAX_OP_ARGS],
-                       const int const_args[TCG_MAX_OP_ARGS])
+                       const TCGArg *args, const int *const_args)
 {
     /* 99% of the time, we can signal the use of extension registers
        by looking to see if the opcode handles 64-bit data.  */
diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
index 6775bd30fc..976f0f05af 100644
--- a/tcg/sparc/tcg-target.c.inc
+++ b/tcg/sparc/tcg-target.c.inc
@@ -1294,8 +1294,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data, TCGReg addr,
 }
 
 static void tcg_out_op(TCGContext *s, TCGOpcode opc,
-                       const TCGArg args[TCG_MAX_OP_ARGS],
-                       const int const_args[TCG_MAX_OP_ARGS])
+                       const TCGArg *args, const int *const_args)
 {
     TCGArg a0, a1, a2;
     int c, c2;
-- 
2.18.4



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings
  2021-01-11 11:30 [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings mrezanin
                   ` (2 preceding siblings ...)
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning mrezanin
@ 2021-01-11 11:39 ` no-reply
  3 siblings, 0 replies; 17+ messages in thread
From: no-reply @ 2021-01-11 11:39 UTC (permalink / raw)
  To: mrezanin; +Cc: qemu-devel

Patchew URL: https://patchew.org/QEMU/cover.1610364304.git.mrezanin@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: cover.1610364304.git.mrezanin@redhat.com
Subject: [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/cover.1610364304.git.mrezanin@redhat.com -> patchew/cover.1610364304.git.mrezanin@redhat.com
Switched to a new branch 'test'
c05a0c4 Fix tcg_out_op argument mismatch warning
a8b8ace s390x: Fix vm name copy length
b64e754 Fix net.c warning on GCC 11

=== OUTPUT BEGIN ===
1/3 Checking commit b64e754d193f (Fix net.c warning on GCC 11)
2/3 Checking commit a8b8ace484f8 (s390x: Fix vm name copy length)
ERROR: trailing whitespace
#36: FILE: target/s390x/misc_helper.c:372:
+^I^Imemset((char *)sysib.sysib_322.ext_names[0], 0, $

ERROR: code indent should never use tabs
#36: FILE: target/s390x/misc_helper.c:372:
+^I^Imemset((char *)sysib.sysib_322.ext_names[0], 0, $

ERROR: code indent should never use tabs
#37: FILE: target/s390x/misc_helper.c:373:
+^I^I       sizeof(sysib.sysib_322.ext_names[0]));$

total: 3 errors, 0 warnings, 19 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit c05a0c4ed447 (Fix tcg_out_op argument mismatch warning)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 18 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/cover.1610364304.git.mrezanin@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length mrezanin
@ 2021-01-11 12:10   ` Philippe Mathieu-Daudé
  2021-01-11 12:24     ` Thomas Huth
  2021-01-11 12:37     ` Miroslav Rezanina
  0 siblings, 2 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-11 12:10 UTC (permalink / raw)
  To: mrezanin, qemu-devel

Hi Miroslav,

On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> There are two cases when vm name is copied but closing \0 can be lost
> in case name is too long (>=256 characters).
> 
> Updating length to copy so there is space for closing \0.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
>  target/s390x/kvm.c         | 2 +-
>  target/s390x/misc_helper.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index b8385e6b95..2313b5727e 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
>       */
>      if (qemu_name) {
>          strncpy((char *)sysib.ext_names[0], qemu_name,
> -                sizeof(sysib.ext_names[0]));
> +                sizeof(sysib.ext_names[0]) - 1);
>      } else {
>          strcpy((char *)sysib.ext_names[0], "KVMguest");
>      }

What about using strpadcpy() instead?

    strpadcpy((char *)sysib.sysib_322.ext_names[0],
              sizeof(sysib.sysib_322.ext_names[0]),
              qemu_name ?: "KVMguest", '\0');

> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 58dbc023eb..7c478b9e58 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, uint64_t r0, uint64_t r1)
>                  ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name,
>                             MIN(sizeof(sysib.sysib_322.vm[0].name),
>                                 strlen(qemu_name)));
> +		memset((char *)sysib.sysib_322.ext_names[0], 0, 
> +		       sizeof(sysib.sysib_322.ext_names[0]));
>                  strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name,
> -                        sizeof(sysib.sysib_322.ext_names[0]));
> +                        sizeof(sysib.sysib_322.ext_names[0]) - 1);

And here:

               strpadcpy((char *)sysib.sysib_322.ext_names[0],
                         sizeof(sysib.sysib_322.ext_names[0]),
                         qemu_name, '\0');

>              } else {
>                  ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8);
>                  strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest");
> 



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning
  2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning mrezanin
@ 2021-01-11 12:15   ` Philippe Mathieu-Daudé
  2021-01-11 12:40     ` Miroslav Rezanina
  0 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-11 12:15 UTC (permalink / raw)
  To: mrezanin, qemu-devel; +Cc: Richard Henderson

On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> There's prototype mismatch between tcg/tcg.c and tcg/aarch/tcg-target.c.inc:
> 
> tcg.c:
> 
>     static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
>                            const int *const_args);
> 
> tcg-target.c.inc:
> 
>     static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>                            const TCGArg args[TCG_MAX_OP_ARGS],
>                            const int const_args[TCG_MAX_OP_ARGS])
> 
> This missmatch cause warnings on GCC 11:
> 
>     tcg/aarch64/tcg-target.c.inc:1855:37: error: argument 3 of type 'const TCGArg[16]' {aka 'const long unsigned int[16]'} with mismatched bound [-Werror=array-parameter=]
>     tcg/aarch64/tcg-target.c.inc:1856:34: error: argument 4 of type 'const int[16]' with mismatched bound [-Werror=array-parameter=]

TIL. Interesting, compilers are getting smarter :)

> Only architectures with this definition are aarch and sparc. Fixing both archs to use
> proper argument type.
> ---
>  tcg/aarch64/tcg-target.c.inc | 3 +--
>  tcg/sparc/tcg-target.c.inc   | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
> index 26f71cb599..fe6bdbf721 100644
> --- a/tcg/aarch64/tcg-target.c.inc
> +++ b/tcg/aarch64/tcg-target.c.inc
> @@ -1852,8 +1852,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>  static tcg_insn_unit *tb_ret_addr;
>  
>  static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> -                       const TCGArg args[TCG_MAX_OP_ARGS],
> -                       const int const_args[TCG_MAX_OP_ARGS])
> +                       const TCGArg *args, const int *const_args)

Doing this way we loose information (that the array pointed has
TCG_MAX_OP_ARGS elements). What about letting this prototype and
fix the other uses?

>  {
>      /* 99% of the time, we can signal the use of extension registers
>         by looking to see if the opcode handles 64-bit data.  */
> diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
> index 6775bd30fc..976f0f05af 100644
> --- a/tcg/sparc/tcg-target.c.inc
> +++ b/tcg/sparc/tcg-target.c.inc
> @@ -1294,8 +1294,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data, TCGReg addr,
>  }
>  
>  static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> -                       const TCGArg args[TCG_MAX_OP_ARGS],
> -                       const int const_args[TCG_MAX_OP_ARGS])
> +                       const TCGArg *args, const int *const_args)
>  {
>      TCGArg a0, a1, a2;
>      int c, c2;
> 



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 12:10   ` Philippe Mathieu-Daudé
@ 2021-01-11 12:24     ` Thomas Huth
  2021-01-11 12:42       ` Miroslav Rezanina
  2021-01-11 12:37     ` Miroslav Rezanina
  1 sibling, 1 reply; 17+ messages in thread
From: Thomas Huth @ 2021-01-11 12:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, mrezanin, qemu-devel, qemu-s390x

On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
> Hi Miroslav,
> 
> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>
>> There are two cases when vm name is copied but closing \0 can be lost
>> in case name is too long (>=256 characters).
>>
>> Updating length to copy so there is space for closing \0.
>>
>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>> ---
>>   target/s390x/kvm.c         | 2 +-
>>   target/s390x/misc_helper.c | 4 +++-
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index b8385e6b95..2313b5727e 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
>>        */
>>       if (qemu_name) {
>>           strncpy((char *)sysib.ext_names[0], qemu_name,
>> -                sizeof(sysib.ext_names[0]));
>> +                sizeof(sysib.ext_names[0]) - 1);
>>       } else {
>>           strcpy((char *)sysib.ext_names[0], "KVMguest");
>>       }
> 
> What about using strpadcpy() instead?

Yes, strpadcpy is the better way here - this field has to be padded with 
zeroes, so doing "- 1" is wrong here.

  Thomas



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 12:10   ` Philippe Mathieu-Daudé
  2021-01-11 12:24     ` Thomas Huth
@ 2021-01-11 12:37     ` Miroslav Rezanina
  1 sibling, 0 replies; 17+ messages in thread
From: Miroslav Rezanina @ 2021-01-11 12:37 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel



----- Original Message -----
> From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> To: mrezanin@redhat.com, qemu-devel@nongnu.org
> Sent: Monday, January 11, 2021 1:10:07 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> 
> Hi Miroslav,
> 
> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> > From: Miroslav Rezanina <mrezanin@redhat.com>
> > 
> > There are two cases when vm name is copied but closing \0 can be lost
> > in case name is too long (>=256 characters).
> > 
> > Updating length to copy so there is space for closing \0.
> > 
> > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> > ---
> >  target/s390x/kvm.c         | 2 +-
> >  target/s390x/misc_helper.c | 4 +++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index b8385e6b95..2313b5727e 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
> > addr, uint8_t ar)
> >       */
> >      if (qemu_name) {
> >          strncpy((char *)sysib.ext_names[0], qemu_name,
> > -                sizeof(sysib.ext_names[0]));
> > +                sizeof(sysib.ext_names[0]) - 1);
> >      } else {
> >          strcpy((char *)sysib.ext_names[0], "KVMguest");
> >      }
> 
> What about using strpadcpy() instead?
> 
>     strpadcpy((char *)sysib.sysib_322.ext_names[0],
>               sizeof(sysib.sysib_322.ext_names[0]),
>               qemu_name ?: "KVMguest", '\0');

Hi Philippe, 

I went with -1 here because code use memset to 0 few lines above this code, so
we now there are only zeroes in the array and we do not have to write them again.

> 
> > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> > index 58dbc023eb..7c478b9e58 100644
> > --- a/target/s390x/misc_helper.c
> > +++ b/target/s390x/misc_helper.c
> > @@ -369,8 +369,10 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
> > uint64_t r0, uint64_t r1)
> >                  ebcdic_put(sysib.sysib_322.vm[0].name, qemu_name,
> >                             MIN(sizeof(sysib.sysib_322.vm[0].name),
> >                                 strlen(qemu_name)));
> > +		memset((char *)sysib.sysib_322.ext_names[0], 0,
> > +		       sizeof(sysib.sysib_322.ext_names[0]));
> >                  strncpy((char *)sysib.sysib_322.ext_names[0], qemu_name,
> > -                        sizeof(sysib.sysib_322.ext_names[0]));
> > +                        sizeof(sysib.sysib_322.ext_names[0]) - 1);
> 
> And here:
> 
>                strpadcpy((char *)sysib.sysib_322.ext_names[0],
>                          sizeof(sysib.sysib_322.ext_names[0]),
>                          qemu_name, '\0');

However, here we are adding memset so using strpadcpy is better choice to
ensure we have \0 after name string.

Mirek

> 
> >              } else {
> >                  ebcdic_put(sysib.sysib_322.vm[0].name, "TCGguest", 8);
> >                  strcpy((char *)sysib.sysib_322.ext_names[0], "TCGguest");
> > 
> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning
  2021-01-11 12:15   ` Philippe Mathieu-Daudé
@ 2021-01-11 12:40     ` Miroslav Rezanina
  0 siblings, 0 replies; 17+ messages in thread
From: Miroslav Rezanina @ 2021-01-11 12:40 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Richard Henderson, qemu-devel

----- Original Message -----
> From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> To: mrezanin@redhat.com, qemu-devel@nongnu.org
> Cc: "Richard Henderson" <richard.henderson@linaro.org>, "Eric Blake" <eblake@redhat.com>
> Sent: Monday, January 11, 2021 1:15:04 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning
> 
> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> > From: Miroslav Rezanina <mrezanin@redhat.com>
> > 
> > There's prototype mismatch between tcg/tcg.c and
> > tcg/aarch/tcg-target.c.inc:
> > 
> > tcg.c:
> > 
> >     static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg
> >     *args,
> >                            const int *const_args);
> > 
> > tcg-target.c.inc:
> > 
> >     static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> >                            const TCGArg args[TCG_MAX_OP_ARGS],
> >                            const int const_args[TCG_MAX_OP_ARGS])
> > 
> > This missmatch cause warnings on GCC 11:
> > 
> >     tcg/aarch64/tcg-target.c.inc:1855:37: error: argument 3 of type 'const
> >     TCGArg[16]' {aka 'const long unsigned int[16]'} with mismatched bound
> >     [-Werror=array-parameter=]
> >     tcg/aarch64/tcg-target.c.inc:1856:34: error: argument 4 of type 'const
> >     int[16]' with mismatched bound [-Werror=array-parameter=]
> 
> TIL. Interesting, compilers are getting smarter :)
> 
> > Only architectures with this definition are aarch and sparc. Fixing both
> > archs to use
> > proper argument type.
> > ---
> >  tcg/aarch64/tcg-target.c.inc | 3 +--
> >  tcg/sparc/tcg-target.c.inc   | 3 +--
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
> > index 26f71cb599..fe6bdbf721 100644
> > --- a/tcg/aarch64/tcg-target.c.inc
> > +++ b/tcg/aarch64/tcg-target.c.inc
> > @@ -1852,8 +1852,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg
> > data_reg, TCGReg addr_reg,
> >  static tcg_insn_unit *tb_ret_addr;
> >  
> >  static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> > -                       const TCGArg args[TCG_MAX_OP_ARGS],
> > -                       const int const_args[TCG_MAX_OP_ARGS])
> > +                       const TCGArg *args, const int *const_args)
> 
> Doing this way we loose information (that the array pointed has
> TCG_MAX_OP_ARGS elements). What about letting this prototype and
> fix the other uses?

I'm not author of the code so I went with smaller change - forward definition
in tcg.c and most tct-target.c.inc use this form. I would need someone more
familiar with this part to clarify whether it's ok to go with opposite change.

Mirek

> 
> >  {
> >      /* 99% of the time, we can signal the use of extension registers
> >         by looking to see if the opcode handles 64-bit data.  */
> > diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
> > index 6775bd30fc..976f0f05af 100644
> > --- a/tcg/sparc/tcg-target.c.inc
> > +++ b/tcg/sparc/tcg-target.c.inc
> > @@ -1294,8 +1294,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg
> > data, TCGReg addr,
> >  }
> >  
> >  static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> > -                       const TCGArg args[TCG_MAX_OP_ARGS],
> > -                       const int const_args[TCG_MAX_OP_ARGS])
> > +                       const TCGArg *args, const int *const_args)
> >  {
> >      TCGArg a0, a1, a2;
> >      int c, c2;
> > 
> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 12:24     ` Thomas Huth
@ 2021-01-11 12:42       ` Miroslav Rezanina
  2021-01-11 12:54         ` Thomas Huth
  0 siblings, 1 reply; 17+ messages in thread
From: Miroslav Rezanina @ 2021-01-11 12:42 UTC (permalink / raw)
  To: Thomas Huth; +Cc: qemu-s390x, Philippe Mathieu-Daudé, qemu-devel



----- Original Message -----
> From: "Thomas Huth" <thuth@redhat.com>
> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com, qemu-devel@nongnu.org, "qemu-s390x"
> <qemu-s390x@nongnu.org>
> Sent: Monday, January 11, 2021 1:24:57 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> 
> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
> > Hi Miroslav,
> > 
> > On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> >> From: Miroslav Rezanina <mrezanin@redhat.com>
> >>
> >> There are two cases when vm name is copied but closing \0 can be lost
> >> in case name is too long (>=256 characters).
> >>
> >> Updating length to copy so there is space for closing \0.
> >>
> >> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> >> ---
> >>   target/s390x/kvm.c         | 2 +-
> >>   target/s390x/misc_helper.c | 4 +++-
> >>   2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index b8385e6b95..2313b5727e 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
> >> addr, uint8_t ar)
> >>        */
> >>       if (qemu_name) {
> >>           strncpy((char *)sysib.ext_names[0], qemu_name,
> >> -                sizeof(sysib.ext_names[0]));
> >> +                sizeof(sysib.ext_names[0]) - 1);
> >>       } else {
> >>           strcpy((char *)sysib.ext_names[0], "KVMguest");
> >>       }
> > 
> > What about using strpadcpy() instead?
> 
> Yes, strpadcpy is the better way here - this field has to be padded with
> zeroes, so doing "- 1" is wrong here.

Hi Thomas,

as I wrote in reply to Phillipe - the array is memset to zeroes before the if so we
are sure it's padded with zeroes (in this occurrence, not true for second one).

Mirek

> 
>   Thomas
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 12:42       ` Miroslav Rezanina
@ 2021-01-11 12:54         ` Thomas Huth
  2021-01-11 12:58           ` Miroslav Rezanina
  2021-01-11 13:02           ` Christian Borntraeger
  0 siblings, 2 replies; 17+ messages in thread
From: Thomas Huth @ 2021-01-11 12:54 UTC (permalink / raw)
  To: Miroslav Rezanina; +Cc: qemu-s390x, Philippe Mathieu-Daudé, qemu-devel

On 11/01/2021 13.42, Miroslav Rezanina wrote:
> 
> 
> ----- Original Message -----
>> From: "Thomas Huth" <thuth@redhat.com>
>> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com, qemu-devel@nongnu.org, "qemu-s390x"
>> <qemu-s390x@nongnu.org>
>> Sent: Monday, January 11, 2021 1:24:57 PM
>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>
>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
>>> Hi Miroslav,
>>>
>>> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
>>>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>>>
>>>> There are two cases when vm name is copied but closing \0 can be lost
>>>> in case name is too long (>=256 characters).
>>>>
>>>> Updating length to copy so there is space for closing \0.
>>>>
>>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>>>> ---
>>>>    target/s390x/kvm.c         | 2 +-
>>>>    target/s390x/misc_helper.c | 4 +++-
>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index b8385e6b95..2313b5727e 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
>>>> addr, uint8_t ar)
>>>>         */
>>>>        if (qemu_name) {
>>>>            strncpy((char *)sysib.ext_names[0], qemu_name,
>>>> -                sizeof(sysib.ext_names[0]));
>>>> +                sizeof(sysib.ext_names[0]) - 1);
>>>>        } else {
>>>>            strcpy((char *)sysib.ext_names[0], "KVMguest");
>>>>        }
>>>
>>> What about using strpadcpy() instead?
>>
>> Yes, strpadcpy is the better way here - this field has to be padded with
>> zeroes, so doing "- 1" is wrong here.
> 
> Hi Thomas,
> 
> as I wrote in reply to Phillipe - the array is memset to zeroes before the if so we
> are sure it's padded with zeroes (in this occurrence, not true for second one).

Ok, but dropping the last character is still wrong here. The ext_names do 
not need to be terminated with a \0 if they have the full length.

  Thomas



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 12:54         ` Thomas Huth
@ 2021-01-11 12:58           ` Miroslav Rezanina
  2021-01-11 13:02           ` Christian Borntraeger
  1 sibling, 0 replies; 17+ messages in thread
From: Miroslav Rezanina @ 2021-01-11 12:58 UTC (permalink / raw)
  To: Thomas Huth; +Cc: qemu-s390x, Philippe Mathieu-Daudé, qemu-devel



----- Original Message -----
> From: "Thomas Huth" <thuth@redhat.com>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: "qemu-s390x" <qemu-s390x@nongnu.org>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
> Sent: Monday, January 11, 2021 1:54:06 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> 
> On 11/01/2021 13.42, Miroslav Rezanina wrote:
> > 
> > 
> > ----- Original Message -----
> >> From: "Thomas Huth" <thuth@redhat.com>
> >> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com,
> >> qemu-devel@nongnu.org, "qemu-s390x"
> >> <qemu-s390x@nongnu.org>
> >> Sent: Monday, January 11, 2021 1:24:57 PM
> >> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> >>
> >> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
> >>> Hi Miroslav,
> >>>
> >>> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> >>>> From: Miroslav Rezanina <mrezanin@redhat.com>
> >>>>
> >>>> There are two cases when vm name is copied but closing \0 can be lost
> >>>> in case name is too long (>=256 characters).
> >>>>
> >>>> Updating length to copy so there is space for closing \0.
> >>>>
> >>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> >>>> ---
> >>>>    target/s390x/kvm.c         | 2 +-
> >>>>    target/s390x/misc_helper.c | 4 +++-
> >>>>    2 files changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>> index b8385e6b95..2313b5727e 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
> >>>> addr, uint8_t ar)
> >>>>         */
> >>>>        if (qemu_name) {
> >>>>            strncpy((char *)sysib.ext_names[0], qemu_name,
> >>>> -                sizeof(sysib.ext_names[0]));
> >>>> +                sizeof(sysib.ext_names[0]) - 1);
> >>>>        } else {
> >>>>            strcpy((char *)sysib.ext_names[0], "KVMguest");
> >>>>        }
> >>>
> >>> What about using strpadcpy() instead?
> >>
> >> Yes, strpadcpy is the better way here - this field has to be padded with
> >> zeroes, so doing "- 1" is wrong here.
> > 
> > Hi Thomas,
> > 
> > as I wrote in reply to Phillipe - the array is memset to zeroes before the
> > if so we
> > are sure it's padded with zeroes (in this occurrence, not true for second
> > one).
> 
> Ok, but dropping the last character is still wrong here. The ext_names do
> not need to be terminated with a \0 if they have the full length.

Oh, they do not have to end with \0??? I did not know it. In that case we
probably have to use strpadcpy to get rid of the compiler warning.

Mirek
> 
>   Thomas
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 12:54         ` Thomas Huth
  2021-01-11 12:58           ` Miroslav Rezanina
@ 2021-01-11 13:02           ` Christian Borntraeger
  2021-01-11 13:07             ` Christian Borntraeger
  2021-01-11 13:17             ` Miroslav Rezanina
  1 sibling, 2 replies; 17+ messages in thread
From: Christian Borntraeger @ 2021-01-11 13:02 UTC (permalink / raw)
  To: Thomas Huth, Miroslav Rezanina
  Cc: qemu-s390x, Philippe Mathieu-Daudé, qemu-devel



On 11.01.21 13:54, Thomas Huth wrote:
> On 11/01/2021 13.42, Miroslav Rezanina wrote:
>>
>>
>> ----- Original Message -----
>>> From: "Thomas Huth" <thuth@redhat.com>
>>> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com, qemu-devel@nongnu.org, "qemu-s390x"
>>> <qemu-s390x@nongnu.org>
>>> Sent: Monday, January 11, 2021 1:24:57 PM
>>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>>
>>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
>>>> Hi Miroslav,
>>>>
>>>> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
>>>>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>>>>
>>>>> There are two cases when vm name is copied but closing \0 can be lost
>>>>> in case name is too long (>=256 characters).
>>>>>
>>>>> Updating length to copy so there is space for closing \0.
>>>>>
>>>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>>>>> ---
>>>>>    target/s390x/kvm.c         | 2 +-
>>>>>    target/s390x/misc_helper.c | 4 +++-
>>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>> index b8385e6b95..2313b5727e 100644
>>>>> --- a/target/s390x/kvm.c
>>>>> +++ b/target/s390x/kvm.c
>>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
>>>>> addr, uint8_t ar)
>>>>>         */
>>>>>        if (qemu_name) {
>>>>>            strncpy((char *)sysib.ext_names[0], qemu_name,
>>>>> -                sizeof(sysib.ext_names[0]));
>>>>> +                sizeof(sysib.ext_names[0]) - 1);
>>>>>        } else {
>>>>>            strcpy((char *)sysib.ext_names[0], "KVMguest");
>>>>>        }
>>>>
>>>> What about using strpadcpy() instead?
>>>
>>> Yes, strpadcpy is the better way here - this field has to be padded with
>>> zeroes, so doing "- 1" is wrong here.
>>
>> Hi Thomas,
>>
>> as I wrote in reply to Phillipe - the array is memset to zeroes before the if so we
>> are sure it's padded with zeroes (in this occurrence, not true for second one).
> 
> Ok, but dropping the last character is still wrong here. The ext_names do not need to be terminated with a \0 if they have the full length.
The current code is actually correct. We are perfectly fine without the final \n if the string is really 256 bytes.

Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it necessary? No.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 13:02           ` Christian Borntraeger
@ 2021-01-11 13:07             ` Christian Borntraeger
  2021-01-11 13:17             ` Miroslav Rezanina
  1 sibling, 0 replies; 17+ messages in thread
From: Christian Borntraeger @ 2021-01-11 13:07 UTC (permalink / raw)
  To: Thomas Huth, Miroslav Rezanina
  Cc: qemu-s390x, Philippe Mathieu-Daudé, qemu-devel



On 11.01.21 14:02, Christian Borntraeger wrote:
> 
> 
> On 11.01.21 13:54, Thomas Huth wrote:
>> On 11/01/2021 13.42, Miroslav Rezanina wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Thomas Huth" <thuth@redhat.com>
>>>> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com, qemu-devel@nongnu.org, "qemu-s390x"
>>>> <qemu-s390x@nongnu.org>
>>>> Sent: Monday, January 11, 2021 1:24:57 PM
>>>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>>>
>>>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
>>>>> Hi Miroslav,
>>>>>
>>>>> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
>>>>>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>>>>>
>>>>>> There are two cases when vm name is copied but closing \0 can be lost
>>>>>> in case name is too long (>=256 characters).
>>>>>>
>>>>>> Updating length to copy so there is space for closing \0.
>>>>>>
>>>>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>>>>>> ---
>>>>>>    target/s390x/kvm.c         | 2 +-
>>>>>>    target/s390x/misc_helper.c | 4 +++-
>>>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>>> index b8385e6b95..2313b5727e 100644
>>>>>> --- a/target/s390x/kvm.c
>>>>>> +++ b/target/s390x/kvm.c
>>>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
>>>>>> addr, uint8_t ar)
>>>>>>         */
>>>>>>        if (qemu_name) {
>>>>>>            strncpy((char *)sysib.ext_names[0], qemu_name,
>>>>>> -                sizeof(sysib.ext_names[0]));
>>>>>> +                sizeof(sysib.ext_names[0]) - 1);
>>>>>>        } else {
>>>>>>            strcpy((char *)sysib.ext_names[0], "KVMguest");
>>>>>>        }
>>>>>
>>>>> What about using strpadcpy() instead?
>>>>
>>>> Yes, strpadcpy is the better way here - this field has to be padded with
>>>> zeroes, so doing "- 1" is wrong here.
>>>
>>> Hi Thomas,
>>>
>>> as I wrote in reply to Phillipe - the array is memset to zeroes before the if so we
>>> are sure it's padded with zeroes (in this occurrence, not true for second one).
>>
>> Ok, but dropping the last character is still wrong here. The ext_names do not need to be terminated with a \0 if they have the full length.
> The current code is actually correct. We are perfectly fine without the final \n if the string is really 256 bytes.
> 
> Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it necessary? No.

And yes, Thomas is right. The -1 is wrong. 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 13:02           ` Christian Borntraeger
  2021-01-11 13:07             ` Christian Borntraeger
@ 2021-01-11 13:17             ` Miroslav Rezanina
  2021-01-11 13:19               ` Christian Borntraeger
  1 sibling, 1 reply; 17+ messages in thread
From: Miroslav Rezanina @ 2021-01-11 13:17 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Thomas Huth, Philippe Mathieu-Daudé, qemu-s390x, qemu-devel



----- Original Message -----
> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
> To: "Thomas Huth" <thuth@redhat.com>, "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: "qemu-s390x" <qemu-s390x@nongnu.org>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
> Sent: Monday, January 11, 2021 2:02:32 PM
> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> 
> 
> 
> On 11.01.21 13:54, Thomas Huth wrote:
> > On 11/01/2021 13.42, Miroslav Rezanina wrote:
> >>
> >>
> >> ----- Original Message -----
> >>> From: "Thomas Huth" <thuth@redhat.com>
> >>> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com,
> >>> qemu-devel@nongnu.org, "qemu-s390x"
> >>> <qemu-s390x@nongnu.org>
> >>> Sent: Monday, January 11, 2021 1:24:57 PM
> >>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
> >>>
> >>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
> >>>> Hi Miroslav,
> >>>>
> >>>> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
> >>>>> From: Miroslav Rezanina <mrezanin@redhat.com>
> >>>>>
> >>>>> There are two cases when vm name is copied but closing \0 can be lost
> >>>>> in case name is too long (>=256 characters).
> >>>>>
> >>>>> Updating length to copy so there is space for closing \0.
> >>>>>
> >>>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> >>>>> ---
> >>>>>    target/s390x/kvm.c         | 2 +-
> >>>>>    target/s390x/misc_helper.c | 4 +++-
> >>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>>> index b8385e6b95..2313b5727e 100644
> >>>>> --- a/target/s390x/kvm.c
> >>>>> +++ b/target/s390x/kvm.c
> >>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
> >>>>> addr, uint8_t ar)
> >>>>>         */
> >>>>>        if (qemu_name) {
> >>>>>            strncpy((char *)sysib.ext_names[0], qemu_name,
> >>>>> -                sizeof(sysib.ext_names[0]));
> >>>>> +                sizeof(sysib.ext_names[0]) - 1);
> >>>>>        } else {
> >>>>>            strcpy((char *)sysib.ext_names[0], "KVMguest");
> >>>>>        }
> >>>>
> >>>> What about using strpadcpy() instead?
> >>>
> >>> Yes, strpadcpy is the better way here - this field has to be padded with
> >>> zeroes, so doing "- 1" is wrong here.
> >>
> >> Hi Thomas,
> >>
> >> as I wrote in reply to Phillipe - the array is memset to zeroes before the
> >> if so we
> >> are sure it's padded with zeroes (in this occurrence, not true for second
> >> one).
> > 
> > Ok, but dropping the last character is still wrong here. The ext_names do
> > not need to be terminated with a \0 if they have the full length.
> The current code is actually correct. We are perfectly fine without the final
> \n if the string is really 256 bytes.
> 
> Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it
> necessary? No.

Yes, it is necessary because otherwise compiler (GCC 11) produce warning and so
build fail when --enable-werror is used.

Mirek


> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
  2021-01-11 13:17             ` Miroslav Rezanina
@ 2021-01-11 13:19               ` Christian Borntraeger
  0 siblings, 0 replies; 17+ messages in thread
From: Christian Borntraeger @ 2021-01-11 13:19 UTC (permalink / raw)
  To: Miroslav Rezanina
  Cc: Thomas Huth, Philippe Mathieu-Daudé, qemu-s390x, qemu-devel



On 11.01.21 14:17, Miroslav Rezanina wrote:
> 
> 
> ----- Original Message -----
>> From: "Christian Borntraeger" <borntraeger@de.ibm.com>
>> To: "Thomas Huth" <thuth@redhat.com>, "Miroslav Rezanina" <mrezanin@redhat.com>
>> Cc: "qemu-s390x" <qemu-s390x@nongnu.org>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
>> Sent: Monday, January 11, 2021 2:02:32 PM
>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>
>>
>>
>> On 11.01.21 13:54, Thomas Huth wrote:
>>> On 11/01/2021 13.42, Miroslav Rezanina wrote:
>>>>
>>>>
>>>> ----- Original Message -----
>>>>> From: "Thomas Huth" <thuth@redhat.com>
>>>>> To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, mrezanin@redhat.com,
>>>>> qemu-devel@nongnu.org, "qemu-s390x"
>>>>> <qemu-s390x@nongnu.org>
>>>>> Sent: Monday, January 11, 2021 1:24:57 PM
>>>>> Subject: Re: [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length
>>>>>
>>>>> On 11/01/2021 13.10, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Miroslav,
>>>>>>
>>>>>> On 1/11/21 12:30 PM, mrezanin@redhat.com wrote:
>>>>>>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>>>>>>
>>>>>>> There are two cases when vm name is copied but closing \0 can be lost
>>>>>>> in case name is too long (>=256 characters).
>>>>>>>
>>>>>>> Updating length to copy so there is space for closing \0.
>>>>>>>
>>>>>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>>>>>>> ---
>>>>>>>    target/s390x/kvm.c         | 2 +-
>>>>>>>    target/s390x/misc_helper.c | 4 +++-
>>>>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>>>> index b8385e6b95..2313b5727e 100644
>>>>>>> --- a/target/s390x/kvm.c
>>>>>>> +++ b/target/s390x/kvm.c
>>>>>>> @@ -1918,7 +1918,7 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64
>>>>>>> addr, uint8_t ar)
>>>>>>>         */
>>>>>>>        if (qemu_name) {
>>>>>>>            strncpy((char *)sysib.ext_names[0], qemu_name,
>>>>>>> -                sizeof(sysib.ext_names[0]));
>>>>>>> +                sizeof(sysib.ext_names[0]) - 1);
>>>>>>>        } else {
>>>>>>>            strcpy((char *)sysib.ext_names[0], "KVMguest");
>>>>>>>        }
>>>>>>
>>>>>> What about using strpadcpy() instead?
>>>>>
>>>>> Yes, strpadcpy is the better way here - this field has to be padded with
>>>>> zeroes, so doing "- 1" is wrong here.
>>>>
>>>> Hi Thomas,
>>>>
>>>> as I wrote in reply to Phillipe - the array is memset to zeroes before the
>>>> if so we
>>>> are sure it's padded with zeroes (in this occurrence, not true for second
>>>> one).
>>>
>>> Ok, but dropping the last character is still wrong here. The ext_names do
>>> not need to be terminated with a \0 if they have the full length.
>> The current code is actually correct. We are perfectly fine without the final
>> \n if the string is really 256 bytes.
>>
>> Replacing memset + strncpy with strpadcpy is certainly a good cleanup. Is it
>> necessary? No.
> 
> Yes, it is necessary because otherwise compiler (GCC 11) produce warning and so
> build fail when --enable-werror is used.

Fair enough. But that actually means that the compiler warning is wrong, because
we use strncpy exactly in the way as described (allowing for the final \n to be
missing). But let us use strpadcpy then.


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-01-11 13:22 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 11:30 [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings mrezanin
2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 1/3] Fix net.c warning on GCC 11 mrezanin
2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 2/3] s390x: Fix vm name copy length mrezanin
2021-01-11 12:10   ` Philippe Mathieu-Daudé
2021-01-11 12:24     ` Thomas Huth
2021-01-11 12:42       ` Miroslav Rezanina
2021-01-11 12:54         ` Thomas Huth
2021-01-11 12:58           ` Miroslav Rezanina
2021-01-11 13:02           ` Christian Borntraeger
2021-01-11 13:07             ` Christian Borntraeger
2021-01-11 13:17             ` Miroslav Rezanina
2021-01-11 13:19               ` Christian Borntraeger
2021-01-11 12:37     ` Miroslav Rezanina
2021-01-11 11:30 ` [RHEL7 qemu-kvm PATCH 3/3] Fix tcg_out_op argument mismatch warning mrezanin
2021-01-11 12:15   ` Philippe Mathieu-Daudé
2021-01-11 12:40     ` Miroslav Rezanina
2021-01-11 11:39 ` [RHEL7 qemu-kvm PATCH 0/3] Fixing several GCC 11 warnings no-reply

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.