[libvirt] [PATCH RFC 0/9] Introduce virPoolObj[Table]Ptr data/API's

John Ferlan posted 9 patches 7 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1486830585-23866-1-git-send-email-jferlan@redhat.com
include/libvirt/virterror.h             |    1 +
po/POTFILES.in                          |    5 +
src/Makefile.am                         |   18 +-
src/conf/domain_conf.h                  |    3 +-
src/conf/interface_conf.c               |  170 +--
src/conf/interface_conf.h               |   47 +-
src/conf/network_conf.c                 | 1308 +-----------------
src/conf/network_conf.h                 |  133 +-
src/conf/node_device_conf.c             |  497 +------
src/conf/node_device_conf.h             |   86 +-
src/conf/nwfilter_conf.c                |  381 +-----
src/conf/nwfilter_conf.h                |   69 +-
src/conf/secret_conf.c                  |    3 +-
src/conf/secret_conf.h                  |    2 +-
src/conf/storage_conf.c                 |  925 +------------
src/conf/storage_conf.h                 |  128 +-
src/conf/virinterfaceobj.c              |  205 +++
src/conf/virinterfaceobj.h              |   46 +
src/conf/virnetworkobj.c                | 1166 ++++++++++++++++
src/conf/virnetworkobj.h                |  121 ++
src/conf/virnodedeviceobj.c             |  570 ++++++++
src/conf/virnodedeviceobj.h             |   86 ++
src/conf/virnwfilterobj.c               |  373 +++++
src/conf/virnwfilterobj.h               |   64 +
src/conf/virpoolobj.c                   | 1184 ++++++++++++++++
src/conf/virpoolobj.h                   |  222 +++
src/conf/virsecretobj.c                 |  782 ++++-------
src/conf/virsecretobj.h                 |   92 +-
src/conf/virstorageobj.c                | 1206 +++++++++++++++++
src/conf/virstorageobj.h                |  143 ++
src/interface/interface_backend_netcf.c |    6 +-
src/interface/interface_backend_udev.c  |    5 +-
src/libvirt_private.syms                |  217 +--
src/network/bridge_driver.c             | 1812 +++++++++++++------------
src/network/bridge_driver.h             |   11 +-
src/network/bridge_driver_platform.h    |    3 +-
src/node_device/node_device_driver.c    |  388 +++---
src/node_device/node_device_driver.h    |    2 +-
src/node_device/node_device_hal.c       |   82 +-
src/node_device/node_device_udev.c      |   76 +-
src/nwfilter/nwfilter_driver.c          |  225 ++-
src/nwfilter/nwfilter_gentech_driver.c  |  111 +-
src/nwfilter/nwfilter_gentech_driver.h  |    2 +-
src/nwfilter/nwfilter_tech_driver.h     |    2 +-
src/rpc/gendispatch.pl                  |    5 +-
src/secret/secret_driver.c              |  187 +--
src/storage/storage_backend.h           |   32 +-
src/storage/storage_backend_disk.c      |  309 +++--
src/storage/storage_backend_fs.c        |  145 +-
src/storage/storage_backend_gluster.c   |   52 +-
src/storage/storage_backend_iscsi.c     |   65 +-
src/storage/storage_backend_logical.c   |  177 +--
src/storage/storage_backend_mpath.c     |   37 +-
src/storage/storage_backend_rbd.c       |  115 +-
src/storage/storage_backend_scsi.c      |   57 +-
src/storage/storage_backend_sheepdog.c  |   89 +-
src/storage/storage_backend_vstorage.c  |   47 +-
src/storage/storage_backend_zfs.c       |  109 +-
src/storage/storage_driver.c            | 1952 +++++++++++++-------------
src/storage/storage_driver.h            |    6 +-
src/storage/storage_util.c              |  234 ++--
src/storage/storage_util.h              |   31 +-
src/test/test_driver.c                  | 2257 ++++++++++++++-----------------
src/util/virerror.c                     |    1 +
tests/networkxml2conftest.c             |   18 +-
tests/storagevolxml2argvtest.c          |   16 +-
66 files changed, 10145 insertions(+), 8774 deletions(-)
create mode 100644 src/conf/virinterfaceobj.c
create mode 100644 src/conf/virinterfaceobj.h
create mode 100644 src/conf/virnetworkobj.c
create mode 100644 src/conf/virnetworkobj.h
create mode 100644 src/conf/virnodedeviceobj.c
create mode 100644 src/conf/virnodedeviceobj.h
create mode 100644 src/conf/virnwfilterobj.c
create mode 100644 src/conf/virnwfilterobj.h
create mode 100644 src/conf/virpoolobj.c
create mode 100644 src/conf/virpoolobj.h
create mode 100644 src/conf/virstorageobj.c
create mode 100644 src/conf/virstorageobj.h
[libvirt] [PATCH RFC 0/9] Introduce virPoolObj[Table]Ptr data/API's
Posted by John Ferlan 7 years, 2 months ago
Based of current top of git commit id '5620c6095'... I imagine things
will change while this is on-list, so if there is the desire to create
your own branch that's your starting point!

This is perhaps more than an RFC considering I've added comments
already (at least one reason why the insertions count is high). Still
it's not really complete and ready for prime time, but I figured I'd
put it out there for feedback.

Essentially I've tried to follow the domainobj 'model' by creating
an API to manage the "objects" various drivers have the need to manage.
Currently there are 4 drivers using linked lists of some sort and 4
using hash tables in some manner. This implementation modifies the
linked list abusers to use a common hash table.

The common hash table will have uuid/name accessors. The preference
remains uuid for uniqueness, but there are a couple of tables (node
device, nwfilter) that don't have a uuid and thus name is unique
enough for them.

The genesis for this was recent patches to the storage driver that
were essentially copying many lines of code and just changing the
names to do the same thing. As I looked deeper at that - I found
a mish-mash of object management throughout the drivers, so this
is my attempt to unify that.

The "goal" is to have a common look and feel to the API's so there's
less thought over how a specific implementation does things. Each
trip into the driver and exit can be handled in a common manner
especially with respect to locks/refcnt's.

Since I was splitting things apart - I also split out the object
code/data *_conf.{c,h} into separate vir*obj.{c,h} files. I took
some liberties with adjusting some names, modifying some other things
along the way since the type A personality took over to try and
follow more current coding conventions while I was in the middle
of changing things.

While the ultimate goal is to finish with the domain code - that's
just too busy in order to make adjustments right now. Still at least
that code does use a similar hash table model - so while it will be
painful - it can be done later if/when this is accepted (and perhaps
when we know there's a slow period for posting changes).

John Ferlan (9):
  conf: Modify gendispatch.pl to make ACL argument opaque
  conf: Introduce virPoolObj and virPoolTableObj and PoolObj API's
  nodedev: Convert virNodeDevObj[List] to use virPoolObj[Table]
  interface: Convert virInterfaceObj[List] to use virPoolObj[Table]
  nwfilter: Convert virNWFilterObj[List] to use virPoolObj[Table]
  secret: Convert virSecretObj[List] to use virPoolObj[Table]
  volume: Convert virStorageVolObj[List] to use virPoolObj[Table]
  storage: Convert virStoragePoolObj[List] to use virPoolObj[Table]
  network: Convert virNetworkObj[List] to use virPoolObj[Table]

 include/libvirt/virterror.h             |    1 +
 po/POTFILES.in                          |    5 +
 src/Makefile.am                         |   18 +-
 src/conf/domain_conf.h                  |    3 +-
 src/conf/interface_conf.c               |  170 +--
 src/conf/interface_conf.h               |   47 +-
 src/conf/network_conf.c                 | 1308 +-----------------
 src/conf/network_conf.h                 |  133 +-
 src/conf/node_device_conf.c             |  497 +------
 src/conf/node_device_conf.h             |   86 +-
 src/conf/nwfilter_conf.c                |  381 +-----
 src/conf/nwfilter_conf.h                |   69 +-
 src/conf/secret_conf.c                  |    3 +-
 src/conf/secret_conf.h                  |    2 +-
 src/conf/storage_conf.c                 |  925 +------------
 src/conf/storage_conf.h                 |  128 +-
 src/conf/virinterfaceobj.c              |  205 +++
 src/conf/virinterfaceobj.h              |   46 +
 src/conf/virnetworkobj.c                | 1166 ++++++++++++++++
 src/conf/virnetworkobj.h                |  121 ++
 src/conf/virnodedeviceobj.c             |  570 ++++++++
 src/conf/virnodedeviceobj.h             |   86 ++
 src/conf/virnwfilterobj.c               |  373 +++++
 src/conf/virnwfilterobj.h               |   64 +
 src/conf/virpoolobj.c                   | 1184 ++++++++++++++++
 src/conf/virpoolobj.h                   |  222 +++
 src/conf/virsecretobj.c                 |  782 ++++-------
 src/conf/virsecretobj.h                 |   92 +-
 src/conf/virstorageobj.c                | 1206 +++++++++++++++++
 src/conf/virstorageobj.h                |  143 ++
 src/interface/interface_backend_netcf.c |    6 +-
 src/interface/interface_backend_udev.c  |    5 +-
 src/libvirt_private.syms                |  217 +--
 src/network/bridge_driver.c             | 1812 +++++++++++++------------
 src/network/bridge_driver.h             |   11 +-
 src/network/bridge_driver_platform.h    |    3 +-
 src/node_device/node_device_driver.c    |  388 +++---
 src/node_device/node_device_driver.h    |    2 +-
 src/node_device/node_device_hal.c       |   82 +-
 src/node_device/node_device_udev.c      |   76 +-
 src/nwfilter/nwfilter_driver.c          |  225 ++-
 src/nwfilter/nwfilter_gentech_driver.c  |  111 +-
 src/nwfilter/nwfilter_gentech_driver.h  |    2 +-
 src/nwfilter/nwfilter_tech_driver.h     |    2 +-
 src/rpc/gendispatch.pl                  |    5 +-
 src/secret/secret_driver.c              |  187 +--
 src/storage/storage_backend.h           |   32 +-
 src/storage/storage_backend_disk.c      |  309 +++--
 src/storage/storage_backend_fs.c        |  145 +-
 src/storage/storage_backend_gluster.c   |   52 +-
 src/storage/storage_backend_iscsi.c     |   65 +-
 src/storage/storage_backend_logical.c   |  177 +--
 src/storage/storage_backend_mpath.c     |   37 +-
 src/storage/storage_backend_rbd.c       |  115 +-
 src/storage/storage_backend_scsi.c      |   57 +-
 src/storage/storage_backend_sheepdog.c  |   89 +-
 src/storage/storage_backend_vstorage.c  |   47 +-
 src/storage/storage_backend_zfs.c       |  109 +-
 src/storage/storage_driver.c            | 1952 +++++++++++++-------------
 src/storage/storage_driver.h            |    6 +-
 src/storage/storage_util.c              |  234 ++--
 src/storage/storage_util.h              |   31 +-
 src/test/test_driver.c                  | 2257 ++++++++++++++-----------------
 src/util/virerror.c                     |    1 +
 tests/networkxml2conftest.c             |   18 +-
 tests/storagevolxml2argvtest.c          |   16 +-
 66 files changed, 10145 insertions(+), 8774 deletions(-)
 create mode 100644 src/conf/virinterfaceobj.c
 create mode 100644 src/conf/virinterfaceobj.h
 create mode 100644 src/conf/virnetworkobj.c
 create mode 100644 src/conf/virnetworkobj.h
 create mode 100644 src/conf/virnodedeviceobj.c
 create mode 100644 src/conf/virnodedeviceobj.h
 create mode 100644 src/conf/virnwfilterobj.c
 create mode 100644 src/conf/virnwfilterobj.h
 create mode 100644 src/conf/virpoolobj.c
 create mode 100644 src/conf/virpoolobj.h
 create mode 100644 src/conf/virstorageobj.c
 create mode 100644 src/conf/virstorageobj.h

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 0/9] Introduce virPoolObj[Table]Ptr data/API's
Posted by Michal Privoznik 7 years, 1 month ago
On 02/11/2017 05:29 PM, John Ferlan wrote:
> Based of current top of git commit id '5620c6095'... I imagine things
> will change while this is on-list, so if there is the desire to create
> your own branch that's your starting point!

Yeah, they had changed. Sorry for ignoring this for a long time. I'm
gonna review it but obviously, the master had diverged too much to apply
these cleanly -> a rebased version needs to be sent again.

> 
> This is perhaps more than an RFC considering I've added comments
> already (at least one reason why the insertions count is high). Still
> it's not really complete and ready for prime time, but I figured I'd
> put it out there for feedback.
> 
> Essentially I've tried to follow the domainobj 'model' by creating
> an API to manage the "objects" various drivers have the need to manage.
> Currently there are 4 drivers using linked lists of some sort and 4
> using hash tables in some manner. This implementation modifies the
> linked list abusers to use a common hash table.
> 
> The common hash table will have uuid/name accessors. The preference
> remains uuid for uniqueness, but there are a couple of tables (node
> device, nwfilter) that don't have a uuid and thus name is unique
> enough for them.
> 
> The genesis for this was recent patches to the storage driver that
> were essentially copying many lines of code and just changing the
> names to do the same thing. As I looked deeper at that - I found
> a mish-mash of object management throughout the drivers, so this
> is my attempt to unify that.
> 
> The "goal" is to have a common look and feel to the API's so there's
> less thought over how a specific implementation does things. Each
> trip into the driver and exit can be handled in a common manner
> especially with respect to locks/refcnt's.

Agreed. Ideally we can have just one implementation shared among all the
drivers. But let me check the patches how you get there.

> 
> Since I was splitting things apart - I also split out the object
> code/data *_conf.{c,h} into separate vir*obj.{c,h} files. I took
> some liberties with adjusting some names, modifying some other things
> along the way since the type A personality took over to try and
> follow more current coding conventions while I was in the middle
> of changing things.

Fair enough.

> 
> While the ultimate goal is to finish with the domain code - that's
> just too busy in order to make adjustments right now. Still at least
> that code does use a similar hash table model - so while it will be
> painful - it can be done later if/when this is accepted (and perhaps
> when we know there's a slow period for posting changes).

Yeah, this is going to be tricky. But if we review quickly we might be
lucky to have as few conflicts as possible. Having said that, ACKs in my
review should be read as "this change makes sense" not "green to push".

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list