Looks fine. -Sanjay
On 03/05/09 11:18, William Schumann wrote: > [reissued webrev after merging with other fixes] > Sanjay, > Please see comments below. > > Updated webrev: http://cr.opensolaris.org/~wmsch/bug-5653/ > Saved earlier webrev to: http://cr.opensolaris.org/~wmsch/bug-5653-prior/ > Tested changes under x86 and sparc. > > Also, I fixed a bug in which an install slice number is specified, but > the slice already exists and the manifest does not have an explicit > create action for that slice. Tested specifying/not specifying creation > of install slice, varying whether slice already exists or not. Here is > the udiff of the part of disk_slices.c that I modified to fix this. > It's the 4th change in > http://cr.opensolaris.org/~wmsch/bug-5653/usr/src/lib/liborchestrator/disk_slices.c.udiff.html > > >> @@ -556,19 +510,28 @@ >> * so remove it >> */ >> remove_slice_from_table(slice_id); >> } >> } >> - if (install_slice_id != 0) >> + if (install_slice_id != 0) { >> use_whole_partition_for_slice_0 = B_FALSE; >> + if (install_slice_id >= NDKMAP) { >> + om_debug_print(OM_DBGLVL_ERR, >> + "Invalid install slice id %d specified.\n", >> + install_slice_id); >> + return (B_FALSE); >> + } >> + slice_edit_list[install_slice_id].install = B_TRUE; >> + } >> /* >> - * if no slice to install to was explicitly specified >> + * if install slice doesn't yet exist >> * and the default TI action of using the entire disk or >> partition for >> - * slice 0 is not indicated, >> - * create an install slice 0 using all available space >> + * slice 0 is not indicated >> + * create an install slice using all available space >> */ >> - if (!is_install_slice_specified() && >> !use_whole_partition_for_slice_0) { >> + if (!is_slice_already_in_table(install_slice_id) && >> + !use_whole_partition_for_slice_0) { >> /* create install slice in largest free region */ >> om_debug_print(OM_DBGLVL_INFO, >> "Creating install slice %d in largest free >> region in " >> "partition\n", install_slice_id); >> if (!om_create_slice(install_slice_id, 0, B_TRUE)) { >> > Please look at this change if you can. More comments below. > > Sanjay Nadkarni wrote: >> William Schumann wrote: >>> Sanjay, >>> Unless commented otherwise, I've implemented your suggestions. >>> >>> Also, I noticed a bug in the use of the exit value supplied by >>> pthread_exit(exit_val) for pthread_join(thread, exit_val). >>> The address of an auto variable is handed to pthread_exit(), which >>> is then referenced by pthread_join(). The auto variable no longer >>> exists after the thread exits. I don' t think it is a good >>> assumption that the address will still be valid for pthread_join(). >>> >>> Changed the approach, passing the value directly. Tested with good >>> and error conditions. >>> >>> sanjay nadkarni (Laptop) wrote: >>>> In general note that the format for comments does not match ON >>>> standards. >>>> All comments that are not next to a statement should be: >>>> /* >>>> * Comment here >>>> */ >>>> >>>> We should make sure that our hg nits catches this. >>>> >>>> auto_parse.c: >>>> Having #defines for the the strings that are passed into >>>> get_manifest_element_array would be nice. 267: It's rather >>>> counter intuitive to set partition size to 0 to indicate max size. >>>> Isn't there a better way ? >>>> >>>> >>>> auto_install.c: >>>> 56, 57. no need for additional fprintfs. ANSI C allows concats >>>> via " " >>>> 88: Why was fprintf changed to fputs. >>> The output string has already been formatted. It is also a tiny bit >>> dangerous - if the newly formatted string has formatting conversion >>> specifications such as "%s", fprintf will try to print whatever is >>> on the stack - not good for a generalized function. >> I will buy the argument that since the string is already formatted >> therefore fputs is better. >>>> 479: >>>> 482: Returning here is fine. But there's memory allocated at >>>> 471needs to be freed. Looking thru >>>> the code I see similar issues with other error'ed >>>> returns. A quick check reveals thatl diskname is not used until >>>> line 568 so perhaps it could be moved just above it. >>> In principal, there is indeed heap space to be returned. In >>> practice, early exits from this routine result in auto-installer >>> failures. This could have been better designed with an exit path >>> that frees memory from any pointers (initialized to NULL) that have >>> been set through malloc/calloc. >>> >>> *diskname is used on the lines just following the point of >>> allocation in a logging statement. >> Agreed. But cleaning up memory is good practice and consistent with >> other memory frees in the same segment. I see that the updated code >> review has this change. Thanks. > Added error return exit which should free any heap space occupied. >>>> >>>> disk_slices.c: >>>> 257: Unnecessary change. >>> I don't understand: >>> 257 committed_disk_target->dslices = >>> om_duplicate_slice_info(handle, ds); >>> is the main focus of this function: om_set_slice_info() >> Perhaps it may be the webrev, but what I see is that line >> 300-301(left panel) and line 257 (on the right) are identical except >> that the CR has been removed. Since that line met all the format >> requirements it didn't need to change. It's just a nit. > I consider it useful to take full advantage of source line length, since > more code can fit into a window vertically. I consider it part of > source cleanup. > William >>>> >>>> disk_target.c: >>>> While there is allocate_target_disk_info, there is no >>>> corresponding free_target_disk_info. Symmetry would help. >>>> >>>> om_misc.c: >>>> is there a reason why this cannot be generalized i.e. have it in >>>> the logging library code. Further code the logging code be >>>> extended so as to generalize this so that one can use a flag to >>>> direct the output to either stdout/stderr or both. >>> Added function to liblogsvc.so.1 to accommodate. Changes in >>> ls_api.h, ls_main.c. >>> William >>>> >>>> >>>> -Sanjay >>>> >>>> William Schumann wrote: >>>>> Currently in the AI manifest, if zero is specified as a requested >>>>> size for partitions or slices, the maximum available space will be >>>>> allocated. This is non-intuitive, and for clarity, a keyword >>>>> "max_size" shall be used for RNG elements "slice_size" or >>>>> "partition_size". >>>>> >>>>> http://cr.opensolaris.org/~wmsch/bug-5653/ >>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5653 >>>>> >>>>> The sizes in the RNG file had to be changed from numeric only to >>>>> text. >>>>> >>>>> Other changes include: >>>>> - additional manifest parsing problems logged and manifest parsing >>>>> failure path added >>>>> - removing unnecessary functions to fetch particular manifest >>>>> elements replacing it with a generic routine (auto_parse.c) >>>>> - logging routine added to log and print message to file >>>>> - adding command line options for breakpoints for testing >>>>> - moved common code to disk_target.c from disk_parts.c and >>>>> disk_slices.c, fixing bug with missing disk sector size >>>>> >>>>> Unit testing for slice/partition for x86 + slice for SPARC: >>>>> - invalid values for size (alpha, out of range) >>>>> - varying existing partition layouts testing maximum size >>>>> allocation algorithm >>>>> - slice editing, partition editing, both >>>>> - testing breakpoint code >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >> >
