On 6/26/13 5:47 PM, Stefan Behrens wrote:
> On 06/26/2013 21:55, Zach Brown wrote:
>>> + if (!uuid_root) {
>>> + WARN_ON_ONCE(1);
>>> + ret = -ENOENT;
>>> + goto out;
>>> + }
>>
>> WARN_ON_ONCE specifically returns the condition so that you can write:
>>
>> if (WARN_ON_ONCE(!uuid_root)) {
>> ret = -ENOENT;
>> goto out;
>> }
>>
>>> + while (item_size) {
>>> + u64 data;
>>> +
>>> + read_extent_buffer(eb, &data, offset, sizeof(data));
>>> + data = le64_to_cpu(data);
>>> + if (data == subid) {
>>> + ret = 0;
>>> + break;
>>> + }
>>> + offset += sizeof(data);
>>> + item_size -= sizeof(data);
>>> + }
>>
>> fs/btrfs/uuid-tree.c:81 col 24 warning: cast to restricted __le64
>>
>> There are a few more instances of this. The good news is that fixing
>> the sparse warning makes the code better, too.
>>
>> __le64 data;
>>
>> read_extent_buffer(eb, &data, offset, sizeof(data));
>> if (le64_to_cpu(data) == subid) {
>>
>> Plese make sure the rest of the series doesn't add sparse warnings for
>> Josef to get email about a few seconds after he merges.
>>
>>> +int btrfs_insert_uuid_subvol_item(struct btrfs_trans_handle *trans,
>>> + struct btrfs_root *uuid_root, u8 *uuid,
>>> + u64 subvol_id)
>>> +{
>>> + int ret;
>>> +
>>> + ret = btrfs_uuid_tree_lookup(uuid_root, uuid,
>>> + BTRFS_UUID_KEY_SUBVOL, subvol_id);
>>> + if (ret == -ENOENT)
>>> + ret = btrfs_uuid_tree_add(trans, uuid_root, uuid,
>>> + BTRFS_UUID_KEY_SUBVOL, subvol_id);
>>> + return ret;
>>> +}
>>
>>
>>> +int btrfs_insert_uuid_received_subvol_item(struct btrfs_trans_handle
>>> *trans,
>>> + struct btrfs_root *uuid_root,
>>> + u8 *uuid, u64 subvol_id)
>>> +{
>>> + int ret;
>>> +
>>> + ret = btrfs_uuid_tree_lookup(uuid_root, uuid,
>>> + BTRFS_UUID_KEY_RECEIVED_SUBVOL, subvol_id);
>>> + if (ret == -ENOENT)
>>> + ret = btrfs_uuid_tree_add(trans, uuid_root, uuid,
>>> + BTRFS_UUID_KEY_RECEIVED_SUBVOL,
>>> + subvol_id);
>>> + return ret;
>>> +}
>>
>> Just have callers pass in the key type so we get slightly less enormous
>> function names and less cut-and-paste code.
>
> Thanks for your comments, but this salami review procedure is not very
> efficient. Everything that you comment on now and before is there since V1.
I'm not sure that makes it any less relevant. We'd all like complete & early
reviews, but unfortunately it's a busy, messy world. Sparse will keep
complaining even at V7 w/o fixing it. :) So better late than never, no?
> Please tell me when you are done with the full review. And please also stop
> the bikeshedding.
Catching something new on the 2nd review pass isn't that unusual. I tend to
agree that not cutting & pasting 25 lines is a noble goal (not really
bikeshedding) if all it takes is a key argument to avoid it... fs/btrfs is
already plenty big.
Just my 2 cents.
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
