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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 28, 2017 at 04:39:52PM +0000, Daniel P. Berrange wrote:
On Tue, Feb 28, 2017 at 04:52:31PM +0100, Pavel Hrdina wrote:
On Tue, Feb 28, 2017 at 04:15:42PM +0100, Michal Privoznik wrote:
> On 02/11/2017 05:29 PM, John Ferlan wrote:
> > +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.

I agree that the virPoolObj is not the best way to go.  This is similar to
what I had in mind while reading this series.  We should definitely preserve
all those object as they are.

I had a similar approach in my mind, it would basically follow how the
virDomainObjList is done, however the name would be generic (virObjectList,
virObjectListable or virObjectTable) with general APIs for searching by using
a function pointer, inserting, removing, etc.  And each object type would
implement specific stuff.  This would nicely follow how the real
object-inheritance is done in class oriented languages.

I definitely like the idea of having unified approach how to handle all
lists that we use in our code and this would give you the tool how to do it.
The virPoolObj kind of force you to use it.

FWIW, the virObject framework as it exists today was just the bare
minimum we needed in order to get a basic inheritance system up and
running in libvirt. I rather expected that we would extend it in the
future to add further concepts, inspired/borrowed from glib (which
is what stimulated my creation of virObject in the first place). In
particular I could see

- Interface support - the virObjectListable concept sounds like the
  kind of thing that would be suited to an interface, rather than a
  subclass, as it lets different objects support the API contract
  without forcing a common ancestor which might not be appropriate


This is really interesting and bunch of ideas *how* this would be
implemented pop into my mind.  I like the overall idea of using more
virObject goodness and expanding it so that it can to more.

I thing I get why interfaces are more suitable in some cases.  Having
'lockable'  and 'poolable' interfaces, for example, make it possible to
have classes that are lockable, poolable, both, or none of those things
without having three intermediate classes to derive from.

I do not understand one tiny thing, though.  Why is interface good for
this one particular occasion when every poolable object will always need
to be lockable?  I think in this particular case having
virObjectPoolable derive from virObjectLockable is pretty reasonable.

One more thing.  Can we do something about the static type-safety of
virObject APIs in the future?  The only idea I could come up with is
optional GCC plugin tailored to our usage, but that seems cumbersome.

- Signal support - a generalization of our current event reporting
  system to integrate directly with the object system


- Job control support - similarly to the signal support, we could have
                        generalized job control for any object.  This
                        could be an Interface for Lockable objects.

- Weak references - this is a concept which is useful with dealing
  with event callback registration. GLib uses this concept to
  dynamically kill off signal handlers when an object is disposed.
  This avoids problem we currently have where event handlers keep
  an object alive as long as they are registered, so you leak if
  you forget to unregister everything

In any case, I agree with the idea that it would be desirable to try and
model these pool concepts more rexplicitly in the object system, so that
we can maintain type safety instead of turning lots of things into void*
opaque pointers.

Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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

Attachment: signature.asc
Description: Digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux