Dave,
Thanks for the review.
On 08/24/10 04:34 PM, Dave Miner wrote:
On 08/23/10 07:26 AM, 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/
td_mg.c, 358-384: I realize that the algorithm you're using here of
count, realloc, then walk & copy is the same as used for several other
object types here, but it's really not a great design. It would be
great if we would fix it at least for this case, but would you at
least file a bug against libtd to have the design reconsidered so that
the discovery calls return a count to start with, rather than callers
having to re-count the number of objects returned?
The algorithm of count/realloc/walk & copy that you mention, have you a
suggestion on what this could be re-designed to ?
td_discover returns the count already via setting int pointer paramater
number_found. So the caller does not have to re-count the number of
objects found. The return value for the function itself is an error
code. or TD_E_SUCCESS for successfull discovery. Unless I'm missing
something I don't think a bug is required.
td_zpool.c
84: unnecessary NULL test
Fixed
302, 364, 379, 520: "allocate"
Fixed (spotted by Sue aswell)
367: leaked top_target->name
Fixed
381: seems like this would leak memory from prior iterations if this
failed after the first. It'd also leak top_target->targets and
top_target->name on any failure.
Added call to zpool_target_free here, and fixed zpool_target_free() to
also free target->name and target->health.
522,537,563,595,626: would this leak at least zi? 537 and later
returns also seems to leak zi->attributes.targets, if not others.
recommend a goto to a general cleanup and failure handler for this
function, as there's a lot of allocation going on. seems like use of
zpool_info_free would help?
Adding zpool_info_free() should be sufficient in all cases, and this
caters for freeing targets.
884,1024,1069,1114,1159: no error if == NULL? the number of times this
idiom seems to be repeated indicates that there's perhaps a stronger
utility function lurking
Makes sense I'll attempt to strip out each for into a generic utility
function and handle err return as well, when done I'll post another webrev.
Again thanks for the review.
Matt
Dave
_______________________________________________
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
