Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Received-SPF: pass (athena.apache.org: domain of evgeny.kotkov@visualsvn.com
 designates 209.85.215.46 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20130724132445.A34FD2388994@eris.apache.org>
References: <20130724132445.A34FD2388994@eris.apache.org>
From: Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
Date: Thu, 21 Aug 2014 13:44:49 +0400
Message-ID: 
 <CAP_GPNi7K5tKnZL9qEd2JsDMGk6jOf3_cZmh2hco9nBaonJ7Cw@mail.gmail.com>
Subject: Re: svn commit: r1506545 [1/2] - in
 /subversion/branches/fsfs-improvements/subversion:
 include/ libsvn_fs_fs/
To: Subversion Development <dev@subversion.apache.org>
Cc: commits@subversion.apache.org
Content-Type: text/plain; charset=UTF-8

> Author: stefan2
> Date: Wed Jul 24 13:24:44 2013
> New Revision: 1506545
>
> URL: http://svn.apache.org/r1506545
> Log:
> On the fsfs-improvements branch:  Finally switch to a new FSFS ID API.
>
> This replaces the previous string-based API with one that represents
> IDs as quadruples of <revision,sub-id> pairs of integers.  Thus, it
> now matches the implicit usage of the node_id, branch_id, txn_id and
> rev_offset parts of those IDs.
>
> The patch does four things.  First, it replaces the current API in
> id.*.  Second, it must use the new svn_fs_fs__id_part_t * instead of
> const char * in all FSFS code.  In some places we have to add a level
> of indirection as key parts can now be put on stack instead of being
> pool-allocated but we always pass them along through pointers.
>
> Third, structs using a txn ID will use the new data type as well -
> requiring more code to be updated.  Lastly, we have to update code
> that (de-)serializes IDs or handles ID assignment and increments.

[...]

>  svn_fs_fs__id_eq(const svn_fs_id_t *a,
>                   const svn_fs_id_t *b)
>  {
> -  id_private_t *pvta = a->fsap_data, *pvtb = b->fsap_data;
> +  fs_fs__id_t *id_a = (fs_fs__id_t *)a;
> +  fs_fs__id_t *id_b = (fs_fs__id_t *)b;
>
>    if (a == b)
>      return TRUE;
> -  if (strcmp(pvta->node_id, pvtb->node_id) != 0)
> -     return FALSE;
> -  if (strcmp(pvta->copy_id, pvtb->copy_id) != 0)
> -    return FALSE;
> -  if ((pvta->txn_id == NULL) != (pvtb->txn_id == NULL))
> -    return FALSE;
> -  if (pvta->txn_id && pvtb->txn_id && strcmp(pvta->txn_id, pvtb->txn_id) != 0)
> -    return FALSE;
> -  if (pvta->rev != pvtb->rev)
> -    return FALSE;
> -  if (pvta->offset != pvtb->offset)
> -    return FALSE;
> -  return TRUE;
> +
> +  return memcmp(&id_a->private_id, &id_b->private_id,
> +                sizeof(id_a->private_id)) == 0;
>  }

I am wondering, whether this is a portable way of comparing structs.  Isn't
it possible that fs_fs__id_t would compile with padding and hence, result in
false negative checks here?

It looks like we do not always use apr_pcalloc() to zero out the memory for
these structures (e.g. within the subversion/libsvn_fs_fs/temp_deserializer.c
code), so it might be possible that the padding bytes would contain some random
garbage, and we would attempt to compare it with memcmp().


Regards,
Evgeny Kotkov