[PATCH] multi-process: Initialize variables declared with g_auto*

[PATCH] multi-process: Initialize variables declared with g_auto*

[Zenghui Yu]

Quote docs/devel/style.rst (section "Automatic memory deallocation"):

* Variables declared with g_auto* MUST always be initialized,
  otherwise the cleanup function will use uninitialized stack memory

Initialize @name properly to get rid of the compilation error:

../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
/usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   g_free (*pp);
   ^~~~~~~~~~~~
../hw/remote/proxy.c:350:30: note: 'name' was declared here
             g_autofree char *name;
                              ^~~~

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 hw/remote/memory.c | 3 +--
 hw/remote/proxy.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/remote/memory.c b/hw/remote/memory.c
index 32085b1e05..f5f735c15a 100644
--- a/hw/remote/memory.c
+++ b/hw/remote/memory.c
@@ -43,9 +43,8 @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
     remote_sysmem_reset();
 
     for (region = 0; region < msg->num_fds; region++) {
-        g_autofree char *name;
+        g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix++);
         subregion = g_new(MemoryRegion, 1);
-        name = g_strdup_printf("remote-mem-%u", suffix++);
         memory_region_init_ram_from_fd(subregion, NULL,
                                        name, sysmem_info->sizes[region],
                                        true, msg->fds[region],
diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c
index 4fa4be079d..6dda705fc2 100644
--- a/hw/remote/proxy.c
+++ b/hw/remote/proxy.c
@@ -347,13 +347,12 @@ static void probe_pci_info(PCIDevice *dev, Error **errp)
                    PCI_BASE_ADDRESS_SPACE_IO : PCI_BASE_ADDRESS_SPACE_MEMORY;
 
         if (size) {
-            g_autofree char *name;
+            g_autofree char *name = g_strdup_printf("bar-region-%d", i);
             pdev->region[i].dev = pdev;
             pdev->region[i].present = true;
             if (type == PCI_BASE_ADDRESS_SPACE_MEMORY) {
                 pdev->region[i].memory = true;
             }
-            name = g_strdup_printf("bar-region-%d", i);
             memory_region_init_io(&pdev->region[i].mr, OBJECT(pdev),
                                   &proxy_mr_ops, &pdev->region[i],
                                   name, size);
-- 
2.19.1

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Jag Raman]

> On Mar 3, 2021, at 2:06 AM, Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
> 
> * Variables declared with g_auto* MUST always be initialized,
>  otherwise the cleanup function will use uninitialized stack memory
> 
> Initialize @name properly to get rid of the compilation error:
> 
> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   g_free (*pp);
>   ^~~~~~~~~~~~
> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
>             g_autofree char *name;
>                              ^~~~
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> hw/remote/memory.c | 3 +--
> hw/remote/proxy.c  | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/remote/memory.c b/hw/remote/memory.c
> index 32085b1e05..f5f735c15a 100644
> --- a/hw/remote/memory.c
> +++ b/hw/remote/memory.c
> @@ -43,9 +43,8 @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
>     remote_sysmem_reset();
> 
>     for (region = 0; region < msg->num_fds; region++) {
> -        g_autofree char *name;
> +        g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix++);
>         subregion = g_new(MemoryRegion, 1);
> -        name = g_strdup_printf("remote-mem-%u", suffix++);
>         memory_region_init_ram_from_fd(subregion, NULL,
>                                        name, sysmem_info->sizes[region],
>                                        true, msg->fds[region],
> diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c
> index 4fa4be079d..6dda705fc2 100644
> --- a/hw/remote/proxy.c
> +++ b/hw/remote/proxy.c
> @@ -347,13 +347,12 @@ static void probe_pci_info(PCIDevice *dev, Error **errp)
>                    PCI_BASE_ADDRESS_SPACE_IO : PCI_BASE_ADDRESS_SPACE_MEMORY;
> 
>         if (size) {
> -            g_autofree char *name;
> +            g_autofree char *name = g_strdup_printf("bar-region-%d", i);
>             pdev->region[i].dev = pdev;
>             pdev->region[i].present = true;
>             if (type == PCI_BASE_ADDRESS_SPACE_MEMORY) {
>                 pdev->region[i].memory = true;
>             }
> -            name = g_strdup_printf("bar-region-%d", i);
>             memory_region_init_io(&pdev->region[i].mr, OBJECT(pdev),
>                                   &proxy_mr_ops, &pdev->region[i],
>                                   name, size);
> -- 
> 2.19.1

Reviewed-by: Jagannathan Raman <jag.raman@oracle.com>
> 

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Philippe Mathieu-Daudé]

Hi,

On 3/3/21 8:06 AM, Zenghui Yu wrote:
> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
> 
> * Variables declared with g_auto* MUST always be initialized,
>   otherwise the cleanup function will use uninitialized stack memory
> 
> Initialize @name properly to get rid of the compilation error:
> 
> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    g_free (*pp);
>    ^~~~~~~~~~~~
> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
>              g_autofree char *name;
>                               ^~~~
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  hw/remote/memory.c | 3 +--
>  hw/remote/proxy.c  | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/remote/memory.c b/hw/remote/memory.c
> index 32085b1e05..f5f735c15a 100644
> --- a/hw/remote/memory.c
> +++ b/hw/remote/memory.c
> @@ -43,9 +43,8 @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
>      remote_sysmem_reset();
>  
>      for (region = 0; region < msg->num_fds; region++) {

Could you move the suffix iteration out of the loop?

       for (region = 0; region < msg->num_fds; region++, suffix++) {

> -        g_autofree char *name;
> +        g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix++);
>          subregion = g_new(MemoryRegion, 1);
> -        name = g_strdup_printf("remote-mem-%u", suffix++);
>          memory_region_init_ram_from_fd(subregion, NULL,
>                                         name, sysmem_info->sizes[region],
>                                         true, msg->fds[region],

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Daniel P. Berrangé]

On Wed, Mar 03, 2021 at 03:06:39PM +0800, Zenghui Yu wrote:
> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
> 
> * Variables declared with g_auto* MUST always be initialized,
>   otherwise the cleanup function will use uninitialized stack memory
> 
> Initialize @name properly to get rid of the compilation error:
> 
> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    g_free (*pp);
>    ^~~~~~~~~~~~
> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
>              g_autofree char *name;
>                               ^~~~

This is a bit wierd.  There should only be risk of uninitialized
variable if there is a 'return' or 'goto' statement between the
variable declaration and and initialization, which is not the
case in either scenario here.

What OS distro and compiler + version are you seeing this with ?

Also we seem to be lacking any gitlab CI job to test with the
multiprocess feature enabled

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Philippe Mathieu-Daudé]

On 3/3/21 11:17 AM, Daniel P. Berrangé wrote:
> On Wed, Mar 03, 2021 at 03:06:39PM +0800, Zenghui Yu wrote:
>> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
>>
>> * Variables declared with g_auto* MUST always be initialized,
>>   otherwise the cleanup function will use uninitialized stack memory
>>
>> Initialize @name properly to get rid of the compilation error:
>>
>> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
>> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    g_free (*pp);
>>    ^~~~~~~~~~~~
>> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
>>              g_autofree char *name;
>>                               ^~~~
> 
> This is a bit wierd.  There should only be risk of uninitialized
> variable if there is a 'return' or 'goto' statement between the
> variable declaration and and initialization, which is not the
> case in either scenario here.

See also commit 076b2fadb58 ("gdbstub: fix compiler complaining").

> 
> What OS distro and compiler + version are you seeing this with ?
> 
> Also we seem to be lacking any gitlab CI job to test with the
> multiprocess feature enabled
> 
> Regards,
> Daniel
> 

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Jag Raman]

> On Mar 3, 2021, at 5:17 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Wed, Mar 03, 2021 at 03:06:39PM +0800, Zenghui Yu wrote:
>> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
>> 
>> * Variables declared with g_auto* MUST always be initialized,
>>  otherwise the cleanup function will use uninitialized stack memory
>> 
>> Initialize @name properly to get rid of the compilation error:
>> 
>> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
>> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>   g_free (*pp);
>>   ^~~~~~~~~~~~
>> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
>>             g_autofree char *name;
>>                              ^~~~
> 
> This is a bit wierd.  There should only be risk of uninitialized
> variable if there is a 'return' or 'goto' statement between the
> variable declaration and and initialization, which is not the
> case in either scenario here.
> 
> What OS distro and compiler + version are you seeing this with ?
> 
> Also we seem to be lacking any gitlab CI job to test with the
> multiprocess feature enabled

Hi Daniel,

Concerning gitlab CI, it looks like we are running acceptance tests as
part of it. "acceptance-system-fedora" runs it on fedora.

Is it sufficient to have multiprocess tests as part acceptance tests suite
or do you prefer to have a separate test in gitlab CI?

Thank you!
—
Jag

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Daniel P. Berrangé]

On Wed, Mar 03, 2021 at 02:24:04PM +0000, Jag Raman wrote:
> 
> 
> > On Mar 3, 2021, at 5:17 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > 
> > On Wed, Mar 03, 2021 at 03:06:39PM +0800, Zenghui Yu wrote:
> >> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
> >> 
> >> * Variables declared with g_auto* MUST always be initialized,
> >>  otherwise the cleanup function will use uninitialized stack memory
> >> 
> >> Initialize @name properly to get rid of the compilation error:
> >> 
> >> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
> >> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >>   g_free (*pp);
> >>   ^~~~~~~~~~~~
> >> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
> >>             g_autofree char *name;
> >>                              ^~~~
> > 
> > This is a bit wierd.  There should only be risk of uninitialized
> > variable if there is a 'return' or 'goto' statement between the
> > variable declaration and and initialization, which is not the
> > case in either scenario here.
> > 
> > What OS distro and compiler + version are you seeing this with ?
> > 
> > Also we seem to be lacking any gitlab CI job to test with the
> > multiprocess feature enabled
> 
> Hi Daniel,
> 
> Concerning gitlab CI, it looks like we are running acceptance tests as
> part of it. "acceptance-system-fedora" runs it on fedora.
> 
> Is it sufficient to have multiprocess tests as part acceptance tests suite
> or do you prefer to have a separate test in gitlab CI?

No problem. it is just me getting confused. I was looking for a CI job
with --enable-multiprocess, not realizing it is enabled by default
on Linux in configure


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Jag Raman]

> On Mar 3, 2021, at 9:26 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Wed, Mar 03, 2021 at 02:24:04PM +0000, Jag Raman wrote:
>> 
>> 
>>> On Mar 3, 2021, at 5:17 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>> 
>>> On Wed, Mar 03, 2021 at 03:06:39PM +0800, Zenghui Yu wrote:
>>>> Quote docs/devel/style.rst (section "Automatic memory deallocation"):
>>>> 
>>>> * Variables declared with g_auto* MUST always be initialized,
>>>> otherwise the cleanup function will use uninitialized stack memory
>>>> 
>>>> Initialize @name properly to get rid of the compilation error:
>>>> 
>>>> ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize':
>>>> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>  g_free (*pp);
>>>>  ^~~~~~~~~~~~
>>>> ../hw/remote/proxy.c:350:30: note: 'name' was declared here
>>>>            g_autofree char *name;
>>>>                             ^~~~
>>> 
>>> This is a bit wierd.  There should only be risk of uninitialized
>>> variable if there is a 'return' or 'goto' statement between the
>>> variable declaration and and initialization, which is not the
>>> case in either scenario here.
>>> 
>>> What OS distro and compiler + version are you seeing this with ?
>>> 
>>> Also we seem to be lacking any gitlab CI job to test with the
>>> multiprocess feature enabled
>> 
>> Hi Daniel,
>> 
>> Concerning gitlab CI, it looks like we are running acceptance tests as
>> part of it. "acceptance-system-fedora" runs it on fedora.
>> 
>> Is it sufficient to have multiprocess tests as part acceptance tests suite
>> or do you prefer to have a separate test in gitlab CI?
> 
> No problem. it is just me getting confused. I was looking for a CI job
> with --enable-multiprocess, not realizing it is enabled by default
> on Linux in configure

Thank you for confirming!

—
Jag

> 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH] multi-process: Initialize variables declared with g_auto*

[Zenghui Yu]

On 2021/3/3 18:17, Daniel P. Berrangé wrote:
> This is a bit wierd.  There should only be risk of uninitialized
> variable if there is a 'return' or 'goto' statement between the
> variable declaration and and initialization, which is not the
> case in either scenario here.
> 
> What OS distro and compiler + version are you seeing this with ?

This was noticed when compiling QEMU with gcc-7.3.0 on CentOS.


Thanks,
Zenghui