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
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
