You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Evgeny Kotkov <ev...@visualsvn.com> on 2014/08/21 11:44:49 UTC
Re: svn commit: r1506545 [1/2] - in /subversion/branches/fsfs-improvements/subversion:
include/ libsvn_fs_fs/
> 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
Re: svn commit: r1506545 [1/2] - in /subversion/branches/fsfs-improvements/subversion:
include/ libsvn_fs_fs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Aug 21, 2014 at 1:17 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 21.08.2014 11:33, Branko Čibej wrote:
>
> On 21.08.2014 10:44, Evgeny Kotkov wrote:
>
>
> I am wondering, whether this is a portable way of comparing structs.
>
>
> It is not. The portable way is to use the '==' operator.
>
>
> ... in C++, that is ...
>
> memcmp is not even safe (in general) if the struct is initialized to a
> known value (e.g., with memset to 0), because the compiler is allowed to
> use the struct padding any way it likes; and a function-local struct may
> not even exist on the stack until the memcmp is called, causing the
> compiler to spill its contents onto the stack.
>
> The only really portable way to compare structs is to (recursively)
> compare its members using '=='. That said, I've yet to see a platform where
> memcmp after memset (or calloc) didn't work.
>
>
Changed in r1619358 and 1620602.
Thanks everyone!
-- Stefan^2.
Re: svn commit: r1506545 [1/2] - in /subversion/branches/fsfs-improvements/subversion:
include/ libsvn_fs_fs/
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2014 11:33, Branko Čibej wrote:
> On 21.08.2014 10:44, Evgeny Kotkov wrote:
>>
>> I am wondering, whether this is a portable way of comparing structs.
>
> It is not. The portable way is to use the '==' operator.
... in C++, that is ...
memcmp is not even safe (in general) if the struct is initialized to a
known value (e.g., with memset to 0), because the compiler is allowed to
use the struct padding any way it likes; and a function-local struct may
not even exist on the stack until the memcmp is called, causing the
compiler to spill its contents onto the stack.
The only really portable way to compare structs is to (recursively)
compare its members using '=='. That said, I've yet to see a platform
where memcmp after memset (or calloc) didn't work.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: svn commit: r1506545 [1/2] - in /subversion/branches/fsfs-improvements/subversion:
include/ libsvn_fs_fs/
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2014 10:44, Evgeny Kotkov wrote:
>> 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.
It is not. The portable way is to use the '==' operator.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com