Re: [libvirt] [PATCH v4 3/4] virStorageFileDeinit: don't free metadata used for storage driver access
On Wed, Dec 7, 2016 at 7:39 PM, Peter Krempawrote: > On Tue, Dec 06, 2016 at 22:52:00 +0530, Prasanna Kumar Kalever wrote: >> Let the metadata for storage driver access to remote and local volumes >> be cleaned by its respective driver *Deinit methods. >> >> This will be used in the next patch, which will implement a connection >> cache for/in gluster protocol driver. >> >> Signed-off-by: Prasanna Kumar Kalever >> --- >> src/storage/storage_backend_fs.c | 2 ++ >> src/storage/storage_backend_gluster.c | 3 ++- >> src/storage/storage_driver.c | 5 + >> 3 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/src/storage/storage_backend_fs.c >> b/src/storage/storage_backend_fs.c >> index de0e8d5..0e03e06 100644 >> --- a/src/storage/storage_backend_fs.c >> +++ b/src/storage/storage_backend_fs.c >> @@ -1488,6 +1488,8 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr >> src) >> >> VIR_FREE(priv->canonpath); >> VIR_FREE(priv); >> + >> +VIR_FREE(src->drv); > > Hmm, so this is a global (for the storage driver) data structure for the > initialized file. It contains the function pointers and a pointer to > private data. > > Even after this patch it's still initialized in the global function. > > Also the src->drv object describes the state of a given > virStorageSource. > > I thought you wanted to get rid of this construct and replace it by > something else, but looking at the following patches it's not the case. Something like ? Apologies, I don't understand your intention here, with some clues and if it is needed I can make it better. > > As of such, this change does not make sense. virStorageFileInit(As) > allocates this structure, so virStorageFileDeinit should free it. Okay, will discard this patch Thanks, -- Prasanna > > NACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/4] virStorageFileDeinit: don't free metadata used for storage driver access
On Tue, Dec 06, 2016 at 22:52:00 +0530, Prasanna Kumar Kalever wrote: > Let the metadata for storage driver access to remote and local volumes > be cleaned by its respective driver *Deinit methods. > > This will be used in the next patch, which will implement a connection > cache for/in gluster protocol driver. > > Signed-off-by: Prasanna Kumar Kalever> --- > src/storage/storage_backend_fs.c | 2 ++ > src/storage/storage_backend_gluster.c | 3 ++- > src/storage/storage_driver.c | 5 + > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/src/storage/storage_backend_fs.c > b/src/storage/storage_backend_fs.c > index de0e8d5..0e03e06 100644 > --- a/src/storage/storage_backend_fs.c > +++ b/src/storage/storage_backend_fs.c > @@ -1488,6 +1488,8 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) > > VIR_FREE(priv->canonpath); > VIR_FREE(priv); > + > +VIR_FREE(src->drv); Hmm, so this is a global (for the storage driver) data structure for the initialized file. It contains the function pointers and a pointer to private data. Even after this patch it's still initialized in the global function. Also the src->drv object describes the state of a given virStorageSource. I thought you wanted to get rid of this construct and replace it by something else, but looking at the following patches it's not the case. As of such, this change does not make sense. virStorageFileInit(As) allocates this structure, so virStorageFileDeinit should free it. NACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 3/4] virStorageFileDeinit: don't free metadata used for storage driver access
Let the metadata for storage driver access to remote and local volumes be cleaned by its respective driver *Deinit methods. This will be used in the next patch, which will implement a connection cache for/in gluster protocol driver. Signed-off-by: Prasanna Kumar Kalever--- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 3 ++- src/storage/storage_driver.c | 5 + 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de0e8d5..0e03e06 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1488,6 +1488,8 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) VIR_FREE(priv->canonpath); VIR_FREE(priv); + +VIR_FREE(src->drv); } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0d5b7f6..e0841ca 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -562,7 +562,8 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) VIR_FREE(priv->canonpath); VIR_FREE(priv); -src->drv->priv = NULL; + +VIR_FREE(src->drv); } static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 24e2f35..2a4e160 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2900,11 +2900,8 @@ virStorageFileDeinit(virStorageSourcePtr src) if (!virStorageFileIsInitialized(src)) return; -if (src->drv->backend && -src->drv->backend->backendDeinit) +if (src->drv->backend && src->drv->backend->backendDeinit) src->drv->backend->backendDeinit(src); - -VIR_FREE(src->drv); } -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list