superfluous " else if ()"
/me wonders if this
if (ret >= 0) {
/* Add an item for the type for the first time */
eb = path->nodes[0];
slot = path->slots[0];
offset = btrfs_item_ptr_offset(eb, slot);
} else if (ret == -EEXIST) {
/*
* An item with that type already exists.
* Extend the item and store the new subid at the end.
*/
btrfs_extend_item(uuid_root, path, sizeof(subid_le));
eb = path->nodes[0];
slot = path->slots[0];
offset = btrfs_item_ptr_offset(eb, slot);
offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le);
} else if (ret < 0) { <-----------------
btrfs_warn(uuid_root->fs_info, "insert uuid item failed %d "
"(0x%016llx, 0x%016llx) type %u!",
ret, (unsigned long long)key.objectid,
(unsigned long long)key.offset, type);
goto out;
}shouldn't be condensed into just "} else {" ?
Signature · 1 line
Toralf
Quoted 26 lines · Toralf Förster
/me wonders if this
if (ret >= 0) { /* Add an item for the type for the first time */ eb = path->nodes[0]; slot = path->slots[0]; offset = btrfs_item_ptr_offset(eb, slot); } else if (ret == -EEXIST) { /* * An item with that type already exists. * Extend the item and store the new subid at the end. */ btrfs_extend_item(uuid_root, path, sizeof(subid_le)); eb = path->nodes[0]; slot = path->slots[0]; offset = btrfs_item_ptr_offset(eb, slot); offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le); } else if (ret < 0) { <----------------- btrfs_warn(uuid_root->fs_info, "insert uuid item failed %d " "(0x%016llx, 0x%016llx) type %u!", ret, (unsigned long long)key.objectid, (unsigned long long)key.offset, type); goto out; }
Quoted 1 line
shouldn't be condensed into just "} else {" ?
It's equivalent to the original code, but is easier to read/understand. Do you have there other concerns besides readability?
Quoted 27 lines · David Sterba
Quoted 26 lines · Toralf Förster
/me wonders if this
if (ret >= 0) { /* Add an item for the type for the first time */ eb = path->nodes[0]; slot = path->slots[0]; offset = btrfs_item_ptr_offset(eb, slot); } else if (ret == -EEXIST) { /* * An item with that type already exists. * Extend the item and store the new subid at the end. */ btrfs_extend_item(uuid_root, path, sizeof(subid_le)); eb = path->nodes[0]; slot = path->slots[0]; offset = btrfs_item_ptr_offset(eb, slot); offset += btrfs_item_size_nr(eb, slot) - sizeof(subid_le); } else if (ret < 0) { <----------------- btrfs_warn(uuid_root->fs_info, "insert uuid item failed %d " "(0x%016llx, 0x%016llx) type %u!", ret, (unsigned long long)key.objectid, (unsigned long long)key.offset, type); goto out; }
Quoted 5 lines
Quoted 1 line
shouldn't be condensed into just "} else {" ?
It's equivalent to the original code, but is easier to read/understand. Do you have there other concerns besides readability?
Oh no, if it is intended and b/c it is not in a runtime critical path - no.
But what came into my mind, wouldn't the following avoid such question in future ? :
} else {
/* ret < 0 */Signature · 1 line
Toralf
Quoted 1 line · Toralf Förster
But what came into my mind, wouldn't the following avoid such question in future ? :
Yes and though it's a tiny change, I don't mind a patch for that.
Quoted 2 lines
} else {
/* ret < 0 */The comment is IMHO unnecessary.