Re: [libvirt] [PATCH RFC 2/9] conf: Introduce virPoolObj and virPoolTableObj and PoolObj API's

On 02/11/2017 05:29 PM, John Ferlan wrote: > Introduce a new generic virPoolObj[Ptr] and virPoolObjTable[Ptr] in order > to generically manage driver lists of objects rather than the current > hodgepodge of some drivers using a forward linked list and other drivers > using a single UUID only based hash table to look up objects. > > The concept is to mimic the domain objects which keep objects in a UUID > hash table and a name hash table for faster lookup. There's also generic > API's in order to Lock, Reference, Unreference, and Unlock the objects to > remove the need for a heavy handed driver lock. > > Since these are Hash Table objects rather than List Objects, the structures > and API's have been named thusly. These are all based upon the existing > generic object/class model so allocation/freeing is using existing API's. > > In addition to common structures and API's, as each driver is converted > the driver specific ACL filter API's can be replaced by a generic pool > ACL filter API. > > Each of the API's is designed to have a consistency with respect to returning > a locked and referenced object, so that a virPoolObjEndAPI can be called in > order to dereference and unlock the object. > > The ultimate goal is to have a single way to think about these objects > management so that the various drivers don't have to have special or different > code in order to manage based on the "style" of list/table the driver needs > to manage. > > A _virPoolObj uses a fairly generic structure: > > virObjectLockable parent; > bool flags > virPoolDefPtr pooldef; > void *def; > void *newDef; > virFreeCallback defFreeFunc; > void *privateData; > void (*privateDataFreeFunc)(void *); > > Access to the fields of the object is handled via API's to fetch or set > the various fields rather than allowing the calling driver to manage the > object directly. This ensures a more consistent look and feel to the code > for each driver. > > The "def" and "newDef" field along with the defFreeFunc will point at > the driver's specific vir*DefPtr data and the FreeFunc handles the free > of the data when the object is finally removed. The vir*DefPtr API's will > each need to be modified to take an "void *opaque" and assign that to the > specific driver definition type. > > Most drivers have no need for privateData; however, a few drivers will > need to manage driver specific object private data. For those drivers > additional object management code is generated in order to manage the > Get, Set, and test of the private data. The goal being to have one set > of API's to manage the data rather than drivers directly touching it. > > A _virPoolObjTable is another generic structure: > > virObjectLockable parent; > virPoolObjTableType type; > bool nameOnly; > int hashStart; > virHashTable *objsUuid; > virHashTable *objsName; > > Not every driver supports UUID's, so the UUID table isn't required, but > it is the preferred lookup mechanism. The bool 'nameOnly' will help the > table code make those decisions. > > The various Pool Object Table API's are taken from the current object list > managements and converted to use tables. > > Objects are added to the table allowing for a callback mechanism to handle > driver specific "assignment" options when the object already appears in the > table and the driver would be managing the replacement of the "existing" > object "def". > > There are common API's to Add, Clear, Remove, Search, Iterate, Collect, > List, Clone, and Prune objects. Each uses the same common methodology to > lock the table list, use the virHash* APIs, and unlock the table list > returning the requested data for the operation. Generally that means > a locked and referenced object. The virPoolObjEndAPI will handle the > dereference and unlocking. > > Add new error code for the new poolobj subsystem > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > include/libvirt/virterror.h | 1 + > po/POTFILES.in | 1 + > src/Makefile.am | 5 + > src/conf/virpoolobj.c | 1184 +++++++++++++++++++++++++++++++++++++++++++ > src/conf/virpoolobj.h | 222 ++++++++ > src/libvirt_private.syms | 33 ++ > src/util/virerror.c | 1 + > 7 files changed, 1447 insertions(+) > create mode 100644 src/conf/virpoolobj.c > create mode 100644 src/conf/virpoolobj.h This is rather huge change. But let me see if I can review it. One thing though - I would prefer if this would introduce only the bare minimum needed to transform one driver (say node_device). Any extended APIs can be introduced then when you need them (e.g. I don't see virPoolObjSetAutostart() used until 8/9. That way the initial size of this patch could be brought down and thus easier for review. > > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > index 2efee8f..2174334 100644 > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h > @@ -132,6 +132,7 @@ typedef enum { > > VIR_FROM_PERF = 65, /* Error from perf */ > VIR_FROM_LIBSSH = 66, /* Error from libssh connection transport */ > + VIR_FROM_POOLOBJ = 67, /* Error from the poolobj code */ > > # ifdef VIR_ENUM_SENTINELS > VIR_ERR_DOMAIN_LAST > diff --git a/po/POTFILES.in b/po/POTFILES.in > index 365ea66..95fa9a6 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -42,6 +42,7 @@ src/conf/snapshot_conf.c > src/conf/storage_conf.c > src/conf/virchrdev.c > src/conf/virdomainobjlist.c > +src/conf/virpoolobj.c > src/conf/virsecretobj.c > src/cpu/cpu.c > src/cpu/cpu_arm.c > diff --git a/src/Makefile.am b/src/Makefile.am > index 2f32d41..03a8cf3 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -373,6 +373,10 @@ NWFILTER_CONF_SOURCES = \ > $(NWFILTER_PARAM_CONF_SOURCES) \ > conf/nwfilter_conf.c conf/nwfilter_conf.h > > +# "Generic" poolobj sources > +POOLOBJ_SOURCES = \ > + conf/virpoolobj.h conf/virpoolobj.c > + virPoolObj is not bad, but I might prefer virObjectList or something that suggests this is a list of objects, not an list turned into virObject. I mean, to me "Obj" suffix suggests that I'm dealing with an object (which is true as well), but I think the "list-ness" (made up word describing list quality) is more important here. You know what I mean? > # Storage driver generic impl APIs > STORAGE_CONF_SOURCES = \ > conf/storage_conf.h conf/storage_conf.c > @@ -405,6 +409,7 @@ CONF_SOURCES = \ > $(NETDEV_CONF_SOURCES) \ > $(DOMAIN_CONF_SOURCES) \ > $(OBJECT_EVENT_SOURCES) \ > + $(POOLOBJ_SOURCES) \ > $(DOMAIN_EVENT_SOURCES) \ > $(NETWORK_EVENT_SOURCES) \ > $(STORAGE_EVENT_SOURCES) \ > diff --git a/src/conf/virpoolobj.c b/src/conf/virpoolobj.c > new file mode 100644 > index 0000000..bca6524 > --- /dev/null > +++ b/src/conf/virpoolobj.c > @@ -0,0 +1,1184 @@ > +/* > + * virpoolobj.c: internal pool objects handling > + * > + * Copyright (C) 2016 Red Hat, Inc. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + */ > + > +#include <config.h> > + > +#include "datatypes.h" > +#include "viralloc.h" > +#include "virpoolobj.h" > +#include "virerror.h" > +#include "virlog.h" > +#include "virhash.h" > +#include "virstring.h" > + > +#define VIR_FROM_THIS VIR_FROM_POOLOBJ > + > +VIR_LOG_INIT("conf.virpoolobj"); > + > +VIR_ENUM_DECL(virPoolObjTable); > +VIR_ENUM_IMPL(virPoolObjTable, VIR_POOLOBJTABLE_LAST, > + "nodedev", "interface", "nwfilter", "volume", "block storage", > + "secret", "network", "domain snapshot", "domain") > + > +struct _virPoolDef { > + char *uuid; > + char *name; > +}; > + > +struct _virPoolObj { > + virObjectLockable parent; > + > + /* copy of the def->name and def->uuid (if available) used for lookups > + * without needing to know the object type */ > + virPoolDefPtr pooldef; > + > + /* Boolean states that can be managed by consumer */ > + bool active; /* object is active or not */ > + bool beingRemoved; /* object being prepared for removal */ > + bool autostart; /* object is autostarted */ > + bool persistent; /* object definition is persistent */ > + bool updated; /* object config has been updated in some way */ > + > + /* Boolean states managed by virPoolObj */ > + bool removing; /* true when object has been removed from table(s) */ > + > + /* 'def' is the current config definition. > + * 'newDef' is the next boot configuration. > + * The 'type' value will describe which _vir*Def struct describes > + * The 'privateData' will be private object data for each pool obj > + * and specific to the 'type' of object being managed. */ > + void *def; > + void *newDef; > + virFreeCallback defFreeFunc; > + > + /* Private data to be managed by the specific object using > + * vir{$OBJ}ObjPrivate{Get|Set}{$FIELD} type API's. Each of those > + * calling the virPoolObjGetPrivateData in order to access this field */ > + void *privateData; > + void (*privateDataFreeFunc)(void *); > +}; So we are creating a new object to be put into hash table. How does this work with the actual object then? I mean, take patch 6/9 for instance. virSecretObj is actual object. Now, when dealing with virPoolObjList, it's just virPoolObj which is locked and not the actual virSecretObj. I'm worried that this could cause some trouble. Maybe virSecret is not the best example, because it doesn't usually get unlocked & locked again throughout virSecret APIs. Maybe virDomain would be a better example. A-ha! after going through all the patches, this new virPoolObj is a merge of all the vir*Obj (virSecretObj, virNetworkObj, virNodeDeviceObj, ...). So the issue I'm describing above will never occur. I wonder whether we can change this code so that we don't have to change all those objects to virPoolObj. I mean, our virObject supports morphism therefore we should be able to have APIs that are able to work over different classes derived from virObject. For instance: virSecretObjPtr sec; virNodeDeviceObjPtr node; virObjectListEndAPI(&sec); virObjectListEndAPI(&node); The same way we can call: virObjectLock(sec); virObjectLock((node); currently. This would help us to ensure type safeness when passing args to functions. One approach that comes to my mind right now as I'm writing these lines (read as "I haven't thought it through properly yet") is to create a new type of virObject. Currently we have virObject and virObjectLockable. What if we would have virObjectListable which would be derived from virObjectLockable. All those driver objects (virSecretObj for instance) would be derived from virObjectListable then. And in virObjectListable you could hold all those booleans you need. This would have the benefit of not dropping virSecretObj and thus rewriting half of libvirt.