On 08/25/10 03:22, Matt Keenan wrote:
Sue, thanks for the review comments below :
On 08/23/10 11:15 PM, Sue Sohn wrote:
On 08/23/10 04:26, Matt Keenan wrote:
Thanks Jan for the review, I've updated the webrev to contain your
recommendations. All of which I agree with :-)
New webrev available at :
http://cr.opensolaris.org/~mattman/bug-14004/
cheers
Matt
Hi Matt,
Jan covered several things that I had noticed in the first round, but
here
are a few things from the updated webrev:
td_mg.c
2475 Need a block comment for compare_zpool_objs. I know the other
functions don't have them, but we should start somewhere.
Yep.. will add
2455-2495 - Is there a way to combine compare_partition_objs and
compare_zpool_objs somehow (and maybe compare_os_objs,
compare_disk_objs, and compare_slice_objs too)? The code seems very
similar.
I thought about this when adding the function, the only difference
between the 5 compare functions used is what nvlist attribute name to
use when comparing. Best means of changing this to collapse into a
single function that I can think of would be to add a new struct element
to td_obj, e.g. char *compare_attr_name.
This would be set to the relevant attribute name to be used for
comparison, e.g. for disks : td->compare_attr_name would be set to
TD_DISK_ATTR_NAME. Then we could have one single comparision function
which would refer to td->compare_attr_name instead of the hard coded
name there are the moment.
My only reservation in making this change is it's changing code that's
been tried and tested as working, and is not really part of this fix.
So my choice would be to leave well alone. However if you feel strongly
then I can make the relevant changes.
I think that, for this case, it would be better to condense things. Hopefully,
it won't take too much time to do the testing after the changes and we'll have
better code as a result.
Thanks for doing this extra work,
Sue
td_zpool.c
251-2 Align with other comment lines,
Aligns if tabspace is set to 4 :-), but I've added a few spaces so that
it also aligns if tabspace is set to 8 .
302,364,379,520 allocatte -> allocate
Done
General question: I noticed that on 429, you include the name of the
function in the debug msg, but on lines 438, 451 (and several others)
you don't. Any reason? Seems the function name would always be handy to
have.
Acutally it was my intention to add to all of them, well spotted.
test_td.c
1261 - add new option 'z' to comment
Done
Thanks
Matt
Thanks,
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
