Not really a bad exemple. It's a safety net, you release the object (which
doesn't means you free it, but simply that you don't need it anymore in
this particular context). You then set it to NULL to be sure you don't use
it anymore in that particular block of code. As Gilles said an harmless
overkill, that can save your life in same cases.
George.
On Thu, Feb 12, 2015 at 10:33 AM, Ralph Castain <r...@open-mpi.org> wrote:
> True - but to directly answer Adrian’s question:
>
> Setting the buffer to NULL is not necessary and definitely a bad example.
>
>
> > On Feb 12, 2015, at 3:01 AM, Gilles Gouaillardet <
> gilles.gouaillar...@iferc.org> wrote:
> >
> > Adrian,
> >
> > in the case of ompi/group/group_init.c, new_group = NULL is clearly an
> > overkill,
> > but there is nothing wrong with it :
> > it can only be 1 when OBJ_RELEASE is invoked
> > (and hence new_group is already NULL, so no need to NULLify it a second
> > time)
> >
> > that being said some typing can also be saved by replacing goto
> > error_exit; with return NULL;
> >
> > Cheers,
> >
> > Gilles
> >
> > On 2015/02/12 18:48, Adrian Reber wrote:
> >> I am not 100% sure I was understood correctly and I am also not sure I
> >> understand the discussion I triggered.
> >>
> >> Being not very familiar with the Open MPI code base I often look at
> >> other places in the code for examples how something can/could be done.
> >> Looking at different examples OBJ_RELEASE() I see at some places first a
> >> OBJ_RELEASE() and then the buffer is set to NULL.
> >>
> >> pcregrep -r -M 'OBJ_RELEASE.*(\n|.).*=(\s)?NULL' *
> >>
> >> [...]
> >> ompi/group/group_init.c: OBJ_RELEASE (new_group);
> >> new_group = NULL;
> >> ompi/group/group_init.c: OBJ_RELEASE (new_group);
> >> new_group = NULL;
> >> ompi/group/group_init.c: OBJ_RELEASE(new_group);
> >> new_group = NULL;
> >> ompi/group/group_init.c: OBJ_RELEASE (new_group);
> >> new_group = NULL;
> >> ompi/group/group_init.c: OBJ_RELEASE(new_group);
> >> new_group = NULL;
> >> ompi/group/group_init.c: OBJ_RELEASE(new_group);
> >> new_group = NULL;
> >> [... and many more ...]
> >>
> >> That was the reason I was looking at the definition of OBJ_RELEASE() and
> >> I saw it already does set the buffer to NULL. Manually setting it to
> >> NULL could theoretically lead to situation where memory is not correctly
> >> free'd (I have not actually seen it).
> >>
> >> My question is more theoretically that setting the buffer to
> >> NULL is not necessary and a bad example?
> >>
> >> Adrian
> >>
> >> On Thu, Feb 12, 2015 at 12:45:06AM -0800, Ralph Castain wrote:
> >>> It would be good to know where you are seeing this - as was stated,
> the macro reduces the ref count and will NULL the pointer if and only if
> the ref count goes to zero. However, the code may set it to NULL for some
> other reason that relates to the later use of that particular variable.
> >>>
> >>> If not used properly, however, it can lead to a memory leak. So it’s
> best that we (a) identify where this was done (I personally don’t recall
> having seen it), and (b) add comments to the code explaining why it
> explicitly sets the param to NULL (e.g., the object is tracked elsewhere
> and will later be free’d).
> >>>
> >>>
> >>>> On Feb 12, 2015, at 12:09 AM, Adrian Reber <adr...@lisas.de> wrote:
> >>>>
> >>>> I was just curious as if I am calling
> >>>>
> >>>> OBJ_RELEASE(buffer);
> >>>> buffer = NULL;
> >>>>
> >>>> on a buffer with an object count different to 1, the buffer is not
> free'd
> >>>> but set to NULL. If I call it again the buffer is NULL and the
> original
> >>>> buffer will not be free'd. Setting the buffer to NULL seems
> unnecessary.
> >>>>
> >>>> I have not seen this as a problem in the code I was just trying to
> >>>> understand if I have to call only
> >>>>
> >>>> OBJ_RELEASE(buffer);
> >>>>
> >>>> or
> >>>>
> >>>> OBJ_RELEASE(buffer);
> >>>> buffer = NULL;
> >>>>
> >>>> and for me the first variant seems to be the correct one.
> >>>>
> >>>> Adrian
> >>>>
> >>>> On Thu, Feb 12, 2015 at 04:58:02PM +0900, Gilles Gouaillardet wrote:
> >>>>> Adrian,
> >>>>>
> >>>>> opal_obj_update does not fail or success, it returns the new
> >>>>> obj_reference_count.
> >>>>>
> >>>>>
> >>>>> can you point to one specific location in the code where you think
> it is
> >>>>> wrong ?
> >>>>>
> >>>>> OBJ_RELEASE(buffer)
> >>>>> buffer = NULL;
> >>>>>
> >>>>> could be written as
> >>>>>
> >>>>> if (((opal_object_t *)buffer)->obj_reference_count == 1) {
> >>>>> OBJ_RELEASE(buffer);
> >>>>> } else {
> >>>>> buffer = NULL;
> >>>>> }
> >>>>>
> >>>>> that would never ever set buffer to NULL twice, but would be wrong
> >>>>> since there is no atomicity here
> >>>>> /* that was for for the "unnecessary" part */
> >>>>>
> >>>>> about the "wrong" part, why do you think the else branch is wrong ?
> >>>>> /* i mean setting a pointer to NULL is not necessarily wrong */
> >>>>>
> >>>>> Cheers,
> >>>>>
> >>>>> Gilles
> >>>>>
> >>>>>
> >>>>> On 2015/02/12 16:41, Adrian Reber wrote:
> >>>>>> At many places all over the code I see
> >>>>>>
> >>>>>> OBJ_RELEASE(buffer)
> >>>>>> buffer = NULL;
> >>>>>>
> >>>>>> Looking at the definition of OBJ_RELEASE() this seems unnecessary
> and
> >>>>>> wrong:
> >>>>>>
> >>>>>> #define OBJ_RELEASE(object)
> \
> >>>>>> do {
> \
> >>>>>> if (0 == opal_obj_update((opal_object_t *) (object), -1)) {
> \
> >>>>>> opal_obj_run_destructors((opal_object_t *) (object));
> \
> >>>>>> free(object);
> \
> >>>>>> object = NULL;
> \
> >>>>>> }
> \
> >>>>>> } while (0)
> >>>>>>
> >>>>>> The object is set to NULL by the macro and only if the
> opal_obj_update() was
> >>>>>> successful. So it seems setting the buffer manually to NULL after
> OBJ_RELEASE()
> >>>>>> is unnecessary and if opal_obj_update() failed it also is wrong.
> >>>>>>
> >>>>>> Adrian
> >>>>>> _______________________________________________
> >>>>>> devel mailing list
> >>>>>> de...@open-mpi.org
> >>>>>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >>>>>> Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16970.php
> >>>>> _______________________________________________
> >>>>> devel mailing list
> >>>>> de...@open-mpi.org
> >>>>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >>>>> Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16971.php
> >>>> _______________________________________________
> >>>> devel mailing list
> >>>> de...@open-mpi.org
> >>>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >>>> Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16972.php
> >>> _______________________________________________
> >>> devel mailing list
> >>> de...@open-mpi.org
> >>> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >>> Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16973.php
> >> _______________________________________________
> >> devel mailing list
> >> de...@open-mpi.org
> >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >> Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16974.php
> >
> > _______________________________________________
> > devel mailing list
> > de...@open-mpi.org
> > Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> > Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16976.php
>
> _______________________________________________
> devel mailing list
> de...@open-mpi.org
> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel
> Link to this post:
> http://www.open-mpi.org/community/lists/devel/2015/02/16977.php
