On 2017年10月31日 15:41, Nikolay Borisov wrote:
>
>
> On 31.10.2017 09:35, Qu Wenruo wrote:
>>
>>
>> On 2017年10月31日 15:29, Nikolay Borisov wrote:
>>>
>>>
>>> On 31.10.2017 09:15, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 31.10.2017 06:03, Qu Wenruo wrote:
>>>>> For key type BTRFS_UUID_KEY_SUBVOL or BTRFS_UUID_KEY_RECEIVED_SUBVOL the
>>>>> key objectid and key offset are just half of the UUID.
>>>>>
>>>>> However we just print the key as %llu, which is converted from little
>>>>> endian, not byte order for UUID, nor the traditional 36 bytes human
>>>>> readable uuid format.
>>>>>
>>>>> Although true engineer can easily convert it in their brain, but to
>>>>> make it easier for search, output the result UUID using the 36 chars format.
>>>>>
>>>>> Cc: Misono Tomohiro <misono.tomohiro@xxxxxxxxxxxxxx>
>>>>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>>>>> ---
>>>>> Inspired by UUID related work from Misono.
>>>>> ---
>>>>> print-tree.c | 17 ++++++++++++++---
>>>>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/print-tree.c b/print-tree.c
>>>>> index 3c585e31f1fc..687f871db302 100644
>>>>> --- a/print-tree.c
>>>>> +++ b/print-tree.c
>>>>> @@ -803,14 +803,25 @@ void btrfs_print_key(struct btrfs_disk_key *disk_key)
>>>>> }
>>>>> }
>>>>>
>>>>> -static void print_uuid_item(struct extent_buffer *l, unsigned long offset,
>>>>> - u32 item_size)
>>>>> +static void print_uuid_item(struct extent_buffer *l, int slot,
>>>>> + unsigned long offset, u32 item_size)
>>>>> {
>>>>> + struct btrfs_key key;
>>>>> + char uuid_str[BTRFS_UUID_UNPARSED_SIZE];
>>>>> + u8 uuid[BTRFS_UUID_SIZE];
>>>>> +
>>>>> + /* Reassemble the uuid from key.objecitd and key.offset */
>>>>> + btrfs_item_key_to_cpu(l, &key, slot);
>>>>> + put_unaligned_le64(key.objectid, uuid);
>>>>> + put_unaligned_le64(key.offset, uuid + sizeof(u64));
>>>>
>>>> I don't think this will work on a BE system. Because
>>>> btrfs_item_key_to_cpu take the LE representation on-disk and turns it
>>>> into a cpu representation which might very well be BE. And then you
>>>> essentially reverse it by using put_unaligned_le64 for x86 it works fine
>>>> due to it being a LE system.
>>>
>>> Ok, so looking at one of your other patches and some digging seems to
>>> indicate that btrfs explicitly generates LE uuids so your code is
>>> correct, however it's not obvoious from this patch itself. I suggest to
>>> put either a comment above the put_unaligned or a statement in the
>>> commit message that uuids are always generated in little-endian format
>>
>> Or just skip the key endian converting.
>>
>> Since it's byte order to byte order, just memcpy() disk key, with proper
>> comment seems cleaner.
>>
>> BTW UUID doesn't get affected by endian. Because UUID is not a u128
>> value, but just 16 bytes, like checksum.
>> In csum case, we just use memcpy() and write_extent_buffer() without
>> doing any converting.
>
> It does get affected, for more info you can check out commit
> f9727a17db9b ("uuid: rename uuid types").
Indeed, true UUID is not u8[16], but u32 + u16 + u16 + u16 + u48, so
it's affected by endian.
(Well, things in practice sometimes get different from its
original/formal design)
Anyway, I'll extract the key to uuid convert to btrfs_key_to_uuid(), and
add comment in that function so we don't need to bother this tricky part
any longer.
Thanks for the review,
Qu
> It seems after that little
> endian types really refer to guid as per the commit message and the the
> "one true UUID" is actually BE. Btrfs apparently chose to use little
> endian since the on-disk format uses that. Given this, I do think that
> an explicit statement that btrfs' uuids are LE is necessary.
>
>
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>>
>>>>> + uuid_unparse(uuid, uuid_str);
>>>>> +
>>>>> if (item_size & (sizeof(u64) - 1)) {
>>>>> printf("btrfs: uuid item with illegal size %lu!\n",
>>>>> (unsigned long)item_size);
>>>>> return;
>>>>> }
>>>>> + printf("\t\tuuid %s\n", uuid_str);
>>>>> while (item_size) {
>>>>> __le64 subvol_id;
>>>>>
>>>>> @@ -1297,7 +1308,7 @@ void btrfs_print_leaf(struct btrfs_root *root, struct extent_buffer *eb)
>>>>> break;
>>>>> case BTRFS_UUID_KEY_SUBVOL:
>>>>> case BTRFS_UUID_KEY_RECEIVED_SUBVOL:
>>>>> - print_uuid_item(eb, btrfs_item_ptr_offset(eb, i),
>>>>> + print_uuid_item(eb, i, btrfs_item_ptr_offset(eb, i),
>>>>> btrfs_item_size_nr(eb, i));
>>>>> break;
>>>>> case BTRFS_STRING_ITEM_KEY: {
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
Attachment:
signature.asc
Description: OpenPGP digital signature
