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