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