[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Should svn_fs_fs__noderev_same_rep_key() accept NULL representation_t::uniquifier?

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 15 Dec 2008 15:06:42 +0000

On Mon, 2008-12-15 at 08:49 -0600, Hyrum K. Wright wrote:
> Kouhei Sutou wrote:
> > Hi,
> >
> > I don't know about svn_fs_fs__noderev_same_rep_key() but it
> > causes "Segmentation fault" if a->uniquifier or
> > b->uniquifier is NULL. Should it accepts NULL or not?
>
> Yes. When reading pre-format-4 versions of the file system, the uniquifier will
> be NULL, so we should account for this possibility.

Can you make the doc string for the "uniquifier" field indicate that,
then, please? Presently it doesn't indicate that the field may be null.

> > If it accepts NULL as valid VALUE:
> >
> > Index: subversion/libsvn_fs_fs/fs_fs.c
> > ===================================================================
> > --- subversion/libsvn_fs_fs/fs_fs.c (revision 34710)
> > +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> > @@ -3435,6 +3435,12 @@
> > if (a->revision != b->revision)
> > return FALSE;
> >
> > + if (a->uniquifier == b->uniquifier)
>
> Should we explicitly make sure that these are both equal to NULL?

The comparison a few lines further up of the whole representation_t uses
this form, which allows for the fact that if the pointers are equal, the
objects pointed to are definitely equal.

> > + return TRUE;
> > +
> > + if (a->uniquifier == NULL || b->uniquifier == NULL)
> > + return FALSE;

(And, for brevity, the comparison of the whole representation_t pointers
'a' and 'b' could use this form too:

Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 34710)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -3423,10 +3423,7 @@
   if (a == b)
     return TRUE;

- if (a && (! b))
- return FALSE;
-
- if (b && (! a))
+ if (a == NULL || b == NULL)
     return FALSE;

   if (a->offset != b->offset)
)

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=984500
Received on 2008-12-15 16:07:45 CET

This is an archived mail posted to the Subversion Dev mailing list.