[Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Samuel Thibault posted 1 patch 9 weeks ago
Test checkpatch passed
Test docker-clang@ubuntu passed
Test asan passed
Test docker-mingw@fedora passed
Failed in applying to current master (apply log)
ui/curses.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

[Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Posted by Samuel Thibault 9 weeks ago
cchar_t can contain not only attr and chars fields, but also ext_color.
Initialize the whole structure to zero instead of enumerating fields.

Spotted by Coverity: CID 1399711

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 ui/curses.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ui/curses.c b/ui/curses.c
index d29098db9f..e99fbe3e24 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -75,9 +75,8 @@ static void curses_update(DisplayChangeListener *dcl,
             if (vga_to_curses[ch].chars[0]) {
                 curses_line[x] = vga_to_curses[ch];
             } else {
+                curses_line[x] = (cchar_t) {};
                 curses_line[x].chars[0] = ch;
-                curses_line[x].chars[1] = 0;
-                curses_line[x].attr = 0;
             }
             curses_line[x].attr |= at;
         }
-- 
2.20.1


Re: [Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Posted by Peter Maydell 9 weeks ago
On Fri, 15 Mar 2019 at 08:37, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> cchar_t can contain not only attr and chars fields, but also ext_color.
> Initialize the whole structure to zero instead of enumerating fields.
>
> Spotted by Coverity: CID 1399711
>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  ui/curses.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/ui/curses.c b/ui/curses.c
> index d29098db9f..e99fbe3e24 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -75,9 +75,8 @@ static void curses_update(DisplayChangeListener *dcl,
>              if (vga_to_curses[ch].chars[0]) {
>                  curses_line[x] = vga_to_curses[ch];
>              } else {
> +                curses_line[x] = (cchar_t) {};
>                  curses_line[x].chars[0] = ch;
> -                curses_line[x].chars[1] = 0;
> -                curses_line[x].attr = 0;
>              }
>              curses_line[x].attr |= at;

Does this really need the cast ? {} is supposed to be a
universal initializer.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Posted by Samuel Thibault 9 weeks ago
Peter Maydell, le ven. 15 mars 2019 10:06:48 +0000, a ecrit:
> > +                curses_line[x] = (cchar_t) {};
> >                  curses_line[x].chars[0] = ch;
> > -                curses_line[x].chars[1] = 0;
> > -                curses_line[x].attr = 0;
> >              }
> >              curses_line[x].attr |= at;
> 
> Does this really need the cast ?

Yes, otherwise it is refused by the compiler.

> {} is supposed to be a universal initializer.

When used as initialized (cchar_t foo = {}), yes.  But when used in
assignations, no, one has to cast it.

Samuel

Re: [Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Posted by Peter Maydell 9 weeks ago
On Fri, 15 Mar 2019 at 10:52, Samuel Thibault <samuel.thibault@gnu.org> wrote:
>
> Peter Maydell, le ven. 15 mars 2019 10:06:48 +0000, a ecrit:
> > > +                curses_line[x] = (cchar_t) {};
> > >                  curses_line[x].chars[0] = ch;
> > > -                curses_line[x].chars[1] = 0;
> > > -                curses_line[x].attr = 0;
> > >              }
> > >              curses_line[x].attr |= at;
> >
> > Does this really need the cast ?
>
> Yes, otherwise it is refused by the compiler.
>
> > {} is supposed to be a universal initializer.
>
> When used as initialized (cchar_t foo = {}), yes.  But when used in
> assignations, no, one has to cast it.

Ah -- I hadn't realized that wrinkle; thanks for the explanation.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Posted by Eric Blake 9 weeks ago
On 3/15/19 5:06 AM, Peter Maydell wrote:
> On Fri, 15 Mar 2019 at 08:37, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
>>
>> cchar_t can contain not only attr and chars fields, but also ext_color.
>> Initialize the whole structure to zero instead of enumerating fields.
>>
>> Spotted by Coverity: CID 1399711
>>
>> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>> ---
>>  ui/curses.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/ui/curses.c b/ui/curses.c
>> index d29098db9f..e99fbe3e24 100644
>> --- a/ui/curses.c
>> +++ b/ui/curses.c
>> @@ -75,9 +75,8 @@ static void curses_update(DisplayChangeListener *dcl,
>>              if (vga_to_curses[ch].chars[0]) {
>>                  curses_line[x] = vga_to_curses[ch];
>>              } else {
>> +                curses_line[x] = (cchar_t) {};
>>                  curses_line[x].chars[0] = ch;
>> -                curses_line[x].chars[1] = 0;
>> -                curses_line[x].attr = 0;
>>              }
>>              curses_line[x].attr |= at;
> 
> Does this really need the cast ? {} is supposed to be a
> universal initializer.

Or is it worth using:

curses_line[x] = (cchar_t) {
  .chars[0] = ch,
};

so that all other fields not mentioned are zero-initialized, without
relying on the {} extension?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] curses ui: always initialize all curses_line fields

Posted by Samuel Thibault 9 weeks ago
Eric Blake, le ven. 15 mars 2019 08:02:29 -0500, a ecrit:
> On 3/15/19 5:06 AM, Peter Maydell wrote:
> > On Fri, 15 Mar 2019 at 08:37, Samuel Thibault
> > <samuel.thibault@ens-lyon.org> wrote:
> >>
> >> cchar_t can contain not only attr and chars fields, but also ext_color.
> >> Initialize the whole structure to zero instead of enumerating fields.
> >>
> >> Spotted by Coverity: CID 1399711
> >>
> >> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> >> ---
> >>  ui/curses.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/ui/curses.c b/ui/curses.c
> >> index d29098db9f..e99fbe3e24 100644
> >> --- a/ui/curses.c
> >> +++ b/ui/curses.c
> >> @@ -75,9 +75,8 @@ static void curses_update(DisplayChangeListener *dcl,
> >>              if (vga_to_curses[ch].chars[0]) {
> >>                  curses_line[x] = vga_to_curses[ch];
> >>              } else {
> >> +                curses_line[x] = (cchar_t) {};
> >>                  curses_line[x].chars[0] = ch;
> >> -                curses_line[x].chars[1] = 0;
> >> -                curses_line[x].attr = 0;
> >>              }
> >>              curses_line[x].attr |= at;
> > 
> > Does this really need the cast ? {} is supposed to be a
> > universal initializer.
> 
> Or is it worth using:
> 
> curses_line[x] = (cchar_t) {
>   .chars[0] = ch,
> };
> 
> so that all other fields not mentioned are zero-initialized, without
> relying on the {} extension?

Right, it's no less readable :) I have sent an updated patch.

Samuel