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.
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.
td_zpool.c
251-2 Align with other comment lines
302,364,379,520 allocatte -> allocate
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.
test_td.c
1261 - add new option 'z' to comment
Thanks,
Sue
On 08/19/10 02:18 PM, Jan Damborsky wrote:
Hi Matt,
please see my comments below.
Thank you,
Jan
td_mg.c
-------
just a nit:
368 /* allocate space for all disks plus terminator element */
->
368 /* allocate space for all ZFS pools plus terminator element */
td_zpool.c
----------
44-45 - could we make g_zfs & g_zpools_list variables 'static',
since they do not seem to be used outside of this module ?
66 - comment does not seem to be in sync with real function name
nit:
221 return (df);
->
221 return (NULL);
237 return (B_FALSE);
->
237 return (NULL);
429 zi = malloc(sizeof (zpool_info_t));
...
436 bzero(zi, sizeof (zpool_info_t));
->
429 zi = calloc(1, sizeof (zpool_info_t));
nit:
432 allocatte
->
432 allocate
711, 743, 751, 759, 767, - these checks are redundant, as free(NULL)
behaves
accordingly (it is no-op).
1231 if (is_spare == B_TRUE) {
->
1231 if (is_spare) {
1274 - 1278 - do we need to treat '0' case especially or might
we simplify that as
1275 size_mb = BYTES_TO_MB(zi->attributes.size);
1280-1285 - since size_gb is initialiazed to '0' at line 1272,
those lines could be simplified as
if (size_mb > MB_IN_GB) {
size_gb = MB_TO_GB(size_mb);
size_mb = 0;
}
td_zpool.h
----------
53-54 - could we simplify that macro in following way ?
53 #define BYTES_TO_MB(size) ((size) / (MB_IN_GB * MB_IN_GB))
56 - just to be sure, please enclose macro parameter in parentheses:
56 #define MB_TO_GB(size_mb) ONE_DECIMAL(size_mb / MB_IN_GB)
->
56 #define MB_TO_GB(size_mb) ONE_DECIMAL((size_mb) / MB_IN_GB)
On 08/18/10 05:16 PM, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
