Sue, thanks for the review comments below :
On 08/23/10 11:15 PM, Sue Sohn wrote:
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.
Yep.. will add
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.
I thought about this when adding the function, the only difference
between the 5 compare functions used is what nvlist attribute name to
use when comparing. Best means of changing this to collapse into a
single function that I can think of would be to add a new struct element
to td_obj, e.g. char *compare_attr_name.
This would be set to the relevant attribute name to be used for
comparison, e.g. for disks : td->compare_attr_name would be set to
TD_DISK_ATTR_NAME. Then we could have one single comparision function
which would refer to td->compare_attr_name instead of the hard coded
name there are the moment.
My only reservation in making this change is it's changing code that's
been tried and tested as working, and is not really part of this fix.
So my choice would be to leave well alone. However if you feel strongly
then I can make the relevant changes.
td_zpool.c
251-2 Align with other comment lines,
Aligns if tabspace is set to 4 :-), but I've added a few spaces so that
it also aligns if tabspace is set to 8 .
302,364,379,520 allocatte -> allocate
Done
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.
Acutally it was my intention to add to all of them, well spotted.
test_td.c
1261 - add new option 'z' to comment
Done
Thanks
Matt
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
