Joseph J VLcek wrote: > Evan Layton wrote: >> I need a code review for: >> >> http://cr.opensolaris.org/~evanl/snap_1012/ >> >> This webrev includes the fixes for 1002 and 1012 >> >> http://defect.opensolaris.org/bz/show_bug.cgi?id=1002 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=1012 >> >> Thanks! >> -evan >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > > > Item 1: > -------- > If line 168 "if (err != 0) {" evaluates as true bes_found will be passed > to be_free_list() > > Then is it possible that the new code under > > > 172 if (cb.be_nodes->be_node_name == NULL) { > > could be invoked and be_free_list( bes_found ) could be called again ? > > Will be_free_list() handle that? > > Please check this.
Yes this should have been an "else if" not just an "if" statement. The block of code below that has the same issue. I'll fix that as well. > > > Item 2: > ------- > > if ( ptr != NULL ) used in the new code and if (ptr) used in the old > code. Please make it consistent. > > e.g.: > > 830 if (temp_node->be_node_name != NULL) > 832 if (temp_node->be_root_ds != NULL) > 834 if (temp_node->be_rpool != NULL) > 836 if (temp_node->be_mntpnt != NULL) > > > 838 if (temp_node->be_policy_type) Fixed > > Thanks for the comments! -evan
