[PATCH 0/4] virXXXListAdd: Transfer definition ownership

Michal Privoznik posted 4 patches 1 week, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cover.1637686811.git.mprivozn@redhat.com
src/bhyve/bhyve_driver.c     |  6 ++----
src/ch/ch_driver.c           |  7 ++-----
src/conf/domain_conf.c       |  8 ++++----
src/conf/domain_conf.h       |  2 +-
src/conf/virdomainobjlist.c  | 22 +++++++++++++---------
src/conf/virdomainobjlist.h  |  2 +-
src/conf/virinterfaceobj.c   | 25 +++++++++++++++++++------
src/conf/virinterfaceobj.h   |  2 +-
src/conf/virsecretobj.c      | 26 ++++++++++++++------------
src/conf/virsecretobj.h      |  2 +-
src/conf/virstorageobj.c     | 29 ++++++++++++++++-------------
src/conf/virstorageobj.h     |  2 +-
src/libxl/libxl_domain.c     |  3 +--
src/libxl/libxl_driver.c     | 30 ++++++++++--------------------
src/libxl/libxl_migration.c  |  6 ++----
src/lxc/lxc_driver.c         | 24 ++++++++----------------
src/openvz/openvz_conf.c     |  3 +--
src/openvz/openvz_driver.c   | 10 ++++------
src/qemu/qemu_driver.c       | 30 ++++++++++--------------------
src/qemu/qemu_migration.c    |  3 +--
src/qemu/qemu_snapshot.c     |  9 +++------
src/secret/secret_driver.c   |  4 ++--
src/storage/storage_driver.c |  5 ++---
src/test/test_driver.c       | 31 ++++++++++++-------------------
src/vmware/vmware_conf.c     |  6 ++----
src/vmware/vmware_driver.c   | 10 ++++------
26 files changed, 137 insertions(+), 170 deletions(-)

[PATCH 0/4] virXXXListAdd: Transfer definition ownership

Posted by Michal Privoznik 1 week, 5 days ago
At a lot of places we have the following pattern:

  virXXXDef *def = parseDef();

  if (!(obj = virXXXObjListAdd(def)))
    goto clenaup;
  def = NULL;

 cleanup:
  virXXXDefFree(def);


The 'def = NULL' step is necessary because the ownership of the
definition was transferred onto the object. Well, this approach is
fragile as it relies on developers remembering to set the variable
explicitly.

If the virXXXObjListAdd() would take address of @def then the explicit
set to NULL can be left out.

Please note, I've reworked only a few virXXXObjListAdd() functions to
see whether these are desired or not. If merged, I can post patches for
the rest.

Michal Prívozník (4):
  virInterfaceObjListAssignDef: Transfer definition ownership
  virSecretObjListAdd: Transfer definition ownership
  virStoragePoolObjListAdd: Transfer definition ownership
  virDomainObjListAdd: Transfer definition ownership

 src/bhyve/bhyve_driver.c     |  6 ++----
 src/ch/ch_driver.c           |  7 ++-----
 src/conf/domain_conf.c       |  8 ++++----
 src/conf/domain_conf.h       |  2 +-
 src/conf/virdomainobjlist.c  | 22 +++++++++++++---------
 src/conf/virdomainobjlist.h  |  2 +-
 src/conf/virinterfaceobj.c   | 25 +++++++++++++++++++------
 src/conf/virinterfaceobj.h   |  2 +-
 src/conf/virsecretobj.c      | 26 ++++++++++++++------------
 src/conf/virsecretobj.h      |  2 +-
 src/conf/virstorageobj.c     | 29 ++++++++++++++++-------------
 src/conf/virstorageobj.h     |  2 +-
 src/libxl/libxl_domain.c     |  3 +--
 src/libxl/libxl_driver.c     | 30 ++++++++++--------------------
 src/libxl/libxl_migration.c  |  6 ++----
 src/lxc/lxc_driver.c         | 24 ++++++++----------------
 src/openvz/openvz_conf.c     |  3 +--
 src/openvz/openvz_driver.c   | 10 ++++------
 src/qemu/qemu_driver.c       | 30 ++++++++++--------------------
 src/qemu/qemu_migration.c    |  3 +--
 src/qemu/qemu_snapshot.c     |  9 +++------
 src/secret/secret_driver.c   |  4 ++--
 src/storage/storage_driver.c |  5 ++---
 src/test/test_driver.c       | 31 ++++++++++++-------------------
 src/vmware/vmware_conf.c     |  6 ++----
 src/vmware/vmware_driver.c   | 10 ++++------
 26 files changed, 137 insertions(+), 170 deletions(-)

-- 
2.32.0

Re: [PATCH 0/4] virXXXListAdd: Transfer definition ownership

Posted by Martin Kletzander 1 week, 4 days ago
On Tue, Nov 23, 2021 at 06:09:36PM +0100, Michal Privoznik wrote:
>At a lot of places we have the following pattern:
>
>  virXXXDef *def = parseDef();
>
>  if (!(obj = virXXXObjListAdd(def)))
>    goto clenaup;
>  def = NULL;
>
> cleanup:
>  virXXXDefFree(def);
>
>
>The 'def = NULL' step is necessary because the ownership of the
>definition was transferred onto the object. Well, this approach is
>fragile as it relies on developers remembering to set the variable
>explicitly.
>
>If the virXXXObjListAdd() would take address of @def then the explicit
>set to NULL can be left out.
>
>Please note, I've reworked only a few virXXXObjListAdd() functions to
>see whether these are desired or not. If merged, I can post patches for
>the rest.
>
>Michal Prívozník (4):
>  virInterfaceObjListAssignDef: Transfer definition ownership
>  virSecretObjListAdd: Transfer definition ownership
>  virStoragePoolObjListAdd: Transfer definition ownership
>  virDomainObjListAdd: Transfer definition ownership

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>