@trisk As an author of the FreeBSD change I see the following pluses in it.
It improves the description for `sa_modify_attrs`.
The code restructuring allows to increment `length_idx` in only one place under
a very clear condition. Compare that to the ZoL change where the increment is
done in two places and one of them, the original, is hidden within the array
access.
Also, I do not understand why the original code increments `j` in the
`SA_REMOVE` case and ZoL keeps that logic. `j` is obviously an index into
`attr_desc` that we are building. So, it seems that the code would leave a hole
(an element with all zero fields) in `attr_desc`. Why? Are we writing beyond
the end of the array in that case? Are we losing the last attribute if the
removed attribute is not it?
The FreeBSD code just skips the attribute being removed and does not create any
hole.
Because of that I was able to assert that `j == attr_count` after the loop.
There are also advantages of the ZoL change.
There is a better comment before the loop.
`SA_REGISTERED_LEN` is assigned to a variable earlier and so the code becomes
more compact.
The change is smaller and so it is easier to understand (which does *not*
imply that the resulting code is easier to read). There is one exception,
though, in the last "hunk" it is not obvious why `buflen` is replaced with
`length`. That change is a NOP, I believe.
Having said all of the above, I wouldn't just pick one or the other of the
changes, but I'd try to fuse them and probably improve on top of them.
However, I am not sure if the resulting change would be eligible for the fast
track review via this repository. @ahrens ?
Maybe one of the changes should be imported as-is, indeed, and then further
improvements could be submitted as a follow-up change...
---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/24#issuecomment-152546122
_______________________________________________
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer
