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

Re: [libvirt] [RFC PATCH 1/2] Add virObject.



On Mon, Mar 21, 2011 at 04:03:23PM +0800, Hu Tao wrote:
> On Fri, Mar 18, 2011 at 11:02:25AM +0000, Daniel P. Berrange wrote:
> > On Wed, Mar 16, 2011 at 06:29:52PM +0800, Hu Tao wrote:
> > > virObject is the base struct that manages reference-counting
> > > for all structs that need the ability of reference-counting.
> > > ---
> > >  src/Makefile.am          |    1 +
> > >  src/libvirt_private.syms |    5 ++++
> > >  src/util/object.c        |   55 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  src/util/object.h        |   39 ++++++++++++++++++++++++++++++++
> > >  4 files changed, 100 insertions(+), 0 deletions(-)
> > >  create mode 100644 src/util/object.c
> > >  create mode 100644 src/util/object.h
> > > 
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 645119e..3ebabe9 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -81,6 +81,7 @@ UTIL_SOURCES =							\
> > >  		util/util.c util/util.h				\
> > >  		util/xml.c util/xml.h				\
> > >  		util/virtaudit.c util/virtaudit.h               \
> > > +		util/object.c util/object.h			\
> > 
> > Can you rename this to virobject.c / virobject.h.  NB without
> 
> OK.
> 
> > the 't'.  I want rename the virtaudit/virterror stuff later.
> > Ideally all new files in util/ should have a 'vir' prefix,
> > since our current names are so short & generic they can easily
> > clash and / or cause confusion.  eg, the name of the file should
> > match the name of the APIs inside, so virObjectPtr -> virobject.h
> > 
> > > diff --git a/src/util/object.c b/src/util/object.c
> > > new file mode 100644
> > > index 0000000..31ea27b
> > > --- /dev/null
> > > +++ b/src/util/object.c
> > > +#include <assert.h>
> > 
> > NB, our coding guidelines don't allow use of assert().
> > 
> > You can use  sa_assert() which is a no-op just designed to
> > be detectable by static analysis tools.
> 
> OK.
> 
> > 
> > > +#include "object.h"
> > > +
> > > +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj))
> > > +{
> > > +    if (!free)
> > > +        return -1;
> > 
> > We should raise an error here
> 
> OK.
> 
> > 
> > > +
> > > +    obj->ref = 1;
> > > +    obj->free = free;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +#define virAtomicInc(value) \
> > > +    __sync_add_and_fetch(&value, 1)
> > > +#define virAtomicDec(value) \
> > > +    __sync_sub_and_fetch(&value, 1)
> > 
> > I reckon it would be a good idea to put these into a separate
> > file  'src/util/virtatomic.{h,c}'.
> 
> Agree. Will do it.  Is the `virt' prefix a typo? Since you've
> suggested `vir' prefix for files in util/.

Yes, I mean viratomic.h

> > Also, I think we need to make this more portable, since at least
> > on Windows, I don't think we want to assume use of GCC. I think
> > we should at the very least have an impl for GCC builtins, and
> > an impl for Win32 to start with. If someone wants to do further
> > impls later they can... If you look at glib's gatomic.c you'll
> > see example code for Win32 atomic ops which is LGPLv2+ licensed
> 
> Yes, atomic ops should be portable, not limited just on gcc. Can we
> borrow glib's win32 atomic ops? Will it cause any license conflict?
> Anyway I will have a look at glib's win32 atomic implementation.
> Thanks for the information.

Glib's atomic ops code is LGPLv2+ licensed, so we will be fine borrowing
the relevant bits of it.

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


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