10:24 a.m.
On 02/28/2017 10:15 AM, Michal Privoznik wrote:
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>
> ---
> 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.
Splitting this apart shouldn't be too difficult - it was something that
grew and morphed as I went along and found some new bell or whistle that
needed to be added. Undoing that after getting through the drivers
wasn't high on the list.
>
> 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?
When I see *List* I think linked list - which these aren't. I used
"Table" because it was shorter than HashTable.
I went with "Pool" because in the long run all these driver objects are
just a "Pool" of objects that can be referenced via a hash using a key
such as UUID or Name. In some cases, both are used, while in others
only Name is the key.
Why I got to thinking about any of this was the FSPool changes from
Virtuozzo which copied a lot of StoragePool code that it would seem
could be converged. Oh and if you think a bit more - FSPool was created
from StoragePool and well perhaps now you understand more why "Pool" was
used.
Once I started thinking about the application in other drivers - the
term "Pool" just stuck. I'm fine with changing it, but I really dislike
the term List.
> # 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.
Ironically virSecretObj was the "most recent" to be changed from a
forward linked list to an object model similar to virDomainObj
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.
And virDomainObj was (more or less) the reference point
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);
Not having to change the names would be nice; however, one benefit of
changing the name was having the compiler find all the places I needed
to change. The whole purpose of "hiding" what's in obj is to force usage
of fetcher and setter functions. Not allowing code to know what's in the
object allows one to extend/modify it too without affecting everyone
else that may be using it commonly.
Beyond the eye soreness and review, once the change is done it's easy to
move forward. What's difficult is backporting changes, so yes, I
certainly understand the pros and cons...
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.
BTW virObjectListable is terrible name, but it serves its purpose here.
Michal
Sounds like I need to read up on how that virObject code does things. Of
course I'd say "virObjectPoolable" would be better than Listable.
John