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