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

Re: [libvirt] [PATCH 1/6] Add virObject and virAtomic.



On Thu, Apr 07, 2011 at 03:49:15PM +0800, Hu Tao wrote:
> On Wed, Apr 06, 2011 at 02:19:57PM -0600, Eric Blake wrote:
> > On 04/06/2011 01:19 AM, Hu Tao wrote:
> > > virObject is the base struct that manages reference-counting
> > > for all structs that need the ability of reference-counting.
> > > 
> > > virAtomic provides atomic operations which are thread-safe.
> > > ---
> > >  src/Makefile.am          |    2 +
> > >  src/libvirt_private.syms |    5 ++++
> > >  src/util/viratomic.c     |   46 ++++++++++++++++++++++++++++++++++++++++
> > >  src/util/viratomic.h     |   30 ++++++++++++++++++++++++++
> > >  src/util/virobject.c     |   52 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  src/util/virobject.h     |   39 ++++++++++++++++++++++++++++++++++
> > >  6 files changed, 174 insertions(+), 0 deletions(-)
> > >  create mode 100644 src/util/viratomic.c
> > >  create mode 100644 src/util/viratomic.h
> > >  create mode 100644 src/util/virobject.c
> > >  create mode 100644 src/util/virobject.h
> > 
> > > +++ b/src/libvirt_private.syms
> > > @@ -993,3 +993,8 @@ virXPathUInt;
> > >  virXPathULong;
> > >  virXPathULongHex;
> > >  virXPathULongLong;
> > > +
> > > +# object.h
> > 
> > virobject.h, and float this section to appear before virtaudit.h (hmm,
> 
> Oops, there are always things escape from under your nose:(
> 
> > maybe we should do a preliminary patch to rename that to viraudit.h, so
> > that we aren't mixing vir vs. virt in quite so many places).
> > 
> > > +virObjectInit;
> > > +virObjectRef;
> > > +virObjectUnref;
> > 
> > Missing exports from viratomic.h.
> 
> Why the linker doesn't complain about this this time?
> 
> > 
> > > diff --git a/src/util/viratomic.c b/src/util/viratomic.c
> > > new file mode 100644
> > > index 0000000..629f189
> > > --- /dev/null
> > > +++ b/src/util/viratomic.c
> > > @@ -0,0 +1,46 @@
> > 
> > > +
> > > +#ifdef WIN32
> > > +long virAtomicInc(long *value)
> > > +{
> > > +    return InterlockedIncrement(value);
> > > +}
> > > +
> > > +long virAtomicDec(long *value)
> > > +{
> > > +    return InterlockedDecrement(value);
> > 
> > This is an OS-specific replacement.
> > 
> > > +}
> > > +#else /* WIN32 */
> > > +long virAtomicInc(long *value)
> > > +{
> > > +    return __sync_add_and_fetch(value, 1);
> > 
> > This is a gcc builtin, and will fail to compile with other C99
> 
> Yes.
> 
> > compilers.  Meanwhile, won't the gcc builtin just work for mingw (that
> 
> Not sure about this. I don't have a mingw environment to test, but I
> trust gcc and guess it does.
> 
> > is, no need to use the OS-specific InterlockedIncrement if you have the
> > compiler builtin instead).
> > 
> > I think this file needs three implementations:
> > 
> > #if defined __GNUC__ || <detect Intel's compiler, which also has these>
> >   use compiler builtins of __sync_add_and_fetch
> > #elif defined WIN32
> >   use OS primitives, like InterlockedIncrement
> > #else
> >   we're hosed when it comes to lightweight versions, but we can still
> > implement a heavyweight replacement that uses virMutex
> > #endif
> 
> Agreed, this is a better way.
> 
> > 
> > > +++ b/src/util/viratomic.h
> > > @@ -0,0 +1,30 @@
> > 
> > > +#ifndef __VIR_ATOMIC_H
> > > +#define __VIR_ATOMIC_H
> > > +
> > > +long virAtomicInc(long *value);
> > > +long virAtomicDec(long *value);
> > 
> > Mark both of these ATTRIBUTE_NONNULL(1)
> 
> OK.
> 
> > 
> > I'm debating whether they should also be marked ATTRIBUTE_RETURN_CHECK
> 
> No, there are cases you just want to inc/dec a value but do not check
> the modified value.
> 
> > 
> > > +++ b/src/util/virobject.c
> > > @@ -0,0 +1,52 @@
> > > +
> > > +#include "viratomic.h"
> > > +#include "virobject.h"
> > > +#include "logging.h"
> > > +
> > > +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj))
> > 
> > You should declare a typedef:
> > 
> > typedef void (*virObjectFreer)(virObjectPtr);
> > 
> > then it becomes simpler to read:
> > 
> > int virObjectInit(virObjectPtr obj, virObjectFreer f)
> > 
> > Use a different name (I used freer/f above) to avoid -Wshadow warnings
> > with free().
> 
> OK.
> 
> > 
> > > +{
> > > +    if (!free) {
> > 
> > Especially since shadowing means it's impossible to tell if this
> > statement is always true (the address of free() exists) or conditional
> > (there is a local variable named free shadowing the global function),
> > and context-sensitive code reviews are tougher :)
> > 
> > > +        VIR_ERROR0("method free is required.");
> > > +        return -1;
> > > +    }
> > 
> > Should this function also check and return -1 if obj->free was not NULL
> > on entry (that is, guarantee that you can't initialize an object twice)?
> 
> Agreed. And it is dangerous to initialize an object more than once
> because the ref count may already changed and a second initializaton
> just do the wrong thing.
> 
> > 
> > > +
> > > +    obj->ref = 1;
> > > +    obj->free = free;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +void virObjectRef(virObjectPtr obj)
> > > +{
> > > +    sa_assert(obj->ref > 0);
> > 
> > This is useless.  It only helps static analyzers (like clang),
> > 
> > > +    virAtomicInc(&obj->ref);
> > 
> > but there's nothing to analyze that depends on knowing the value was
> > positive.
> > 
> > I'm debating whether to do checking.  Maybe we should do:
> > 
> > if (virAtomicInc(&obj->ref) < 2)
> 
> This means bad things happened, and it is not enough to just give a
> warning. Think about this:
> 
> two threads visit an object whose memory is corrupted that it's ref
> count becomes 0 but it doesn't get freed.
> 
> thread 1:                          thread 2:
> 
> ref the object
> (ref count becomes 1)
> 
>                                    unref the object
> 				   (ref count becomes 0, object being
> 				    freed)
> 
> do something with
> the object, but it
> is already freed by
> thread 2
> 
> We should catch abnormal ref count by some way(if not sa_assert)
> 
> >   VIR_WARN("invalid call to virObjectRef");
> > 
> > > +void virObjectUnref(virObjectPtr obj)
> > > +{
> > > +    sa_assert(obj->ref > 0);
> > 
> > Again, I'm not sure that this buys anything.  But we may want to do:
> > 
> > int ref = virAtomicDec(&obj->ref);
> > if (ref < 0)
> >   VIR_WARN("invalid call to virObjectUnref");
> > else if (ref == 0)
> >   obj->free(obj)
> > 
> > > +    if (virAtomicDec(&obj->ref) == 0)
> > > +        obj->free(obj);
> > > +}
> > > diff --git a/src/util/virobject.h b/src/util/virobject.h
> > > new file mode 100644
> > > index 0000000..cd7d3e8
> > > --- /dev/null
> > > +++ b/src/util/virobject.h
> > 
> > > +
> > > +typedef struct _virObject virObject;
> > > +typedef virObject *virObjectPtr;
> > > +
> > > +struct _virObject {
> > > +    long ref;
> > > +    void (*free)(virObjectPtr obj);
> > 
> > Is virObjectPtr the right thing to use here?  If we assume that all
> > clients will always have a virObjectPtr as their first member, then they
> > can cast that back into the larger object.  Is void* opaque any better,
> 
> Exactly the way(you have already saw my other patches when I was typing
> this:))
> 
> > although that then it requires a third parameter for virObjectInit?
> > Maybe it's worth some documentation on intended use in this header (am I
> 
> OK.
> 
> > getting this usage right? I haven't looked at how you used it later in
> > the series, but am just guessing):
> > 
> > struct _virFoo {
> >     virObject obj; /* Must be first member */
> >     ...
> > };
> > static void virFooFree(virObjectPtr obj)
> > {
> >     virFooPtr foo = obj;
> >     ...
> > }
> > virFooPtr virFooNew(void)
> > {
> >     virFooPtr foo;
> >     if (VIR_ALLOC(foo) < 0) {
> >         virReportOOMError();
> >         return NULL;
> >     }
> >     virObjectInit(&foo->obj, virFooFree);
> >     ...
> >     return foo;
> > }
> > 
> > 
> > > +};
> > > +
> > > +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj));
> > 
> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK
> > 
> > > +void virObjectRef(virObjectPtr obj);
> > 
> > ATTRIBUTE_NONNULL(1)
> > 
> > Also, should this return the current reference count?  Not all callers
> > will need it, but returning void is harsh if someone might be able to
> > use it.
> 
> No. It is meaningless to return the current reference count, because it
> may already changed by others after the caller reads the returned value
> on which it is not safe to make some choice.

I wondered if you had an updated copy of this single patch, with
the changes from this discussion included ?  Even though we're
still debating the rest of this patch series, I'd like us to commit
this first virAtomic / virObject patch, so I can use it in some
other code I have waiting. So if you are able to re-post just this
first virObject patch that would be very helpful

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]