Re: [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option

[Michael S. Tsirkin]

On Fri, Aug 24, 2018 at 04:13:18PM +0100, Peter Maydell wrote:
> On 20 August 2018 at 21:24, Michael S. Tsirkin <address@hidden> wrote:
> > From: Junyan He <address@hidden>
> >
> > When QEMU emulates vNVDIMM labels and migrates vNVDIMM devices, it
> > needs to know whether the backend storage is a real persistent memory,
> > in order to decide whether special operations should be performed to
> > ensure the data persistence.
> >
> > This boolean option 'pmem' allows users to specify whether the backend
> > storage of memory-backend-file is a real persistent memory. If
> > 'pmem=on', QEMU will set the flag RAM_PMEM in the RAM block of the
> > corresponding memory region. If 'pmem' is set while lack of libpmem
> > support, a error is generated.
> 
> Hi; Coverity reports (CID 1395184) that there is a resource leak
> in an error-exit path in this function:
> 
> > +static void file_memory_backend_set_pmem(Object *o, bool value, Error 
> > **errp)
> > +{
> > +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> > +
> > +    if (host_memory_backend_mr_inited(backend)) {
> > +        error_setg(errp, "cannot change property 'pmem' of %s '%s'",
> > +                   object_get_typename(o),
> > +                   object_get_canonical_path_component(o));
> > +        return;
> > +    }
> > +
> > +#ifndef CONFIG_LIBPMEM
> > +    if (value) {
> > +        Error *local_err = NULL;
> > +        error_setg(&local_err,
> > +                   "Lack of libpmem support while setting the 'pmem=on'"
> > +                   " of %s '%s'. We can't ensure data persistence.",
> > +                   object_get_typename(o),
> > +                   object_get_canonical_path_component(o));
> 
> object_get_canonical_path_component() returns a string which
> must be freed using g_free().
> 
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +#endif
> > +
> > +    fb->is_pmem = value;
> > +}
> 
> thanks
> -- PMM


Like the following? Junyan, could you pls try this one and confirm?

Signed-off-by: Michael S. Tsirkin <address@hidden>

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 2476dcb435..d88125b86e 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool 
value, Error **errp)
 #ifndef CONFIG_LIBPMEM
     if (value) {
         Error *local_err = NULL;
+        char *path = object_get_canonical_path_component(o);
+
         error_setg(&local_err,
                    "Lack of libpmem support while setting the 'pmem=on'"
                    " of %s '%s'. We can't ensure data persistence.",
                    object_get_typename(o),
-                   object_get_canonical_path_component(o));
+                   );
+        g_free(path);
         error_propagate(errp, local_err);
         return;
     }