On 09/30/10 09:40 AM, Matt Keenan wrote:
Dave,
Thanks for getting back to me.
New Webrev posted to :
Webrev :
http://cr.opensolaris.org/~mattman/bug-14004
And new Differential webrev posted to :
http://cr.opensolaris.org/~mattman/bug-14004-import
Just one bug in the changes you made:
td_zpool.c, 643: s/l2cache/spares/
No re-review needed.
Dave
Comments below.
On 09/29/10 05:23 PM, Dave Miner wrote:
On 09/ 9/10 06:40 AM, Matt Keenan wrote:
Looking for a 2nd round code review for this bug :
Bug 14004 : libtd should discover zpools on entire partitions
https://defect.opensolaris.org/bz/show_bug.cgi?id=14004
Webrev :
http://cr.opensolaris.org/~mattman/bug-14004
Sorry for the extreme delay in reviewing the updated version.
td_zpool.c:
436: s/*pool/**zi_list/
Fixed
447: indent seems off here, should be 4?
Indent is a single tab, which webrev extents to 8 spaces by default. Hg
nits does
not report any error WRT to this. Either way I've just concat'd this and
previous line
and it's inside the 80 char bounds, so no more indent needed :)
552,580,613,645: how would zt possibly be NULL if we got past the
check at 545?
A NULL value for zt could be returned from td_zpool_target_allocate(),
e.g. if we
are traversing for initial targets, the list of targets includes logs,
but we don't want to
have the logs included in the target list, therefore NULL is returned.
556: seems like targetcnt should just be uint32_t to begin with.
Fixed
562,594,626: no check for calloc failure...
Fixed
866: not sure I get why you do a replace here rather than checking
this at 803?
I replace the health at 866 with a value from the CONFIG_VDEV_STATS
which I know we have at this point. If the vdev stats call fails (791),
I still add the pool to the list of pools discovered, as setting the
health at 803 guarantees at least some health value is set.
again thanks for the review.
Matt
Dave
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
