subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Evgeny Kotkov <evgeny.kot...@visualsvn.com>
Subject Re: svn commit: r1506545 [1/2] - in /subversion/branches/fsfs-improvements/subversion: include/ libsvn_fs_fs/
Date Thu, 21 Aug 2014 09:44:49 GMT
> 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

Mime
View raw message