Evan,
Thanks for the code review, much appreciated.
Comments below :
On 08/25/10 08:43 PM, Evan Layton wrote:
Hi Matt,
All of the following functions should be renamed to indicate that these
are part of td_zpool and not zpool functions from libzfs. Maybe just
adding td_ to the start of these would be sufficient.
zpool_iter_callback
zpool_info_print
zpool_info_print_list
zpool_info_print_ptrs
zpool_get_attributes
zpool_info_list_add
zpool_info_list_release
zpool_info_free
zpool_discover
zpool_info_ptrs_to_ddm_handle
zpool_info_get_ptrs
zpool_target_allocate
zpool_target_print
These variables should also be renamed:
zpool_target/zpool_target_t
zpool_attributes/zpool_attributes_t
zpool_info/zpool_info_t
I know this sounds a bit like a nit but I think it's really very
confusing to have no distinction between the functions internal to
td_zpool and those from libzfs.
Agreed, makes sense something I had thought of just slipped my mind,
doing as you suggest s/zpool_/td_zpool_/g for all of above.
nit:
Add zpool_target_allocate to the function descriptions at the top of the
file.
I've added all function descriptions to top of td_zpool.c
line 338
we should probably be checking vs->vs_state == VDEV_STATE_HEALTHY before
setting top_target->health to "AVAIL".
Good point, added check
Dave already mentioned some of the possible memory leaks so I'll skip
those. :)
Minor nit:
line 800 should be lined up better with line 799
line 1197 should be lined up with 1196 and 1198
Fixed, I generally set ts=4 when editing with vim so I don't tend to see
these alignment issues.
If we return on line 895 doesn't it leak the memory allocated on line 871?
(no call to free_nvlist_array)
line 1034 appears to have the same issue with the memory allocated on
line 1009.
line 1124 and 1169 same issue.
It also looks like tmptarget from line 1021 doesn't get freed on any of
the error conditions before line 1048. However I may just be misreading
that…
tmptarget's scope is only within the for loop, but targets itself should
be freed before the error return, see the next comment for resolution.
Shouldn't logs allocated on line 1054 be freed before returning at line
1079?
All of these were more or less pointed out by dave. I've rewritten this
section and now have a utility function which frees memory in the
correct relevant places. At least I think so pending more reviews of course.
Make sure to run cstyle -pvP as it appears there may be some indentation
issues it could flag. Also you may want to run lint on this even though
this doesn't necessarily have to be lint clean it may help point out
some potential issues.
I tend to run hg nits and make lint, and pretty sure hg nits does cstyle
checking.
cheers
Matt
Thanks!
-evan
On 8/18/10 9:16 AM, Matt Keenan wrote:
Hi,
Requesting expert eyes to code review new enhancement to libtd for zpool
discovery :
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/
This fix implements zpool discovery within libtd. Examples of how to
use can be
seen within test_td.c.
Functionally libtd does not change, discovery of zpools is done in the
same
manner as other types of targets e.g. via API td_discover(), etc.
except for zpools target type of TD_OT_ZPOOL is used.
Please read section 3;2.5 of design doc :
http://hub.opensolaris.org/bin/download/Project+caiman/auto_install/ai-multi-design-0.4.pdf
cheers
Matt
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
