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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list