Sundar, Please find my responses inline. Sundar Yamunachari wrote: > William Schumann wrote: >> please look for the slim_source code webrev in the public location: >> http://cr.opensolaris.org/~wmsch/extended_partition/ >> ... > William: > > usr/src/cmd/auto-install/auto_install.c: > 488: Can you print "true" or "false" based on the value of > 'partition_is_logical' to make the output more readable? OK > > usr/src/cmd/auto-install/auto_parse.c: > 35: You don't need to include sys/dktp/fdisk.h, it is included in > liborchestrator_api.h, which is included in auto_install.h OK > 269, 273: The debug statements look similar (not same). Do you need both? Will delete second. > 264, 313, 434: Do you really want to exit if errno != 0? Why can't you > set errno = 0 before calling strtoull(). The man page to strtoull() > recommends setting errno to 0 before calling the function. You are referring to assert(errno != 0); First of all, this only aborts for debugged versions (not compiled with -DNDEBUG), not production versions. See man assert(3c). This will be a no-op for production code. I added the assert()s to detect a condition that I saw early in development, but could not reproduce. Now, I agree that errno should be set to zero so that the logic following strtoull works correctly. Jan and I discussed the value of using assert as opposed to logging a warning message. We also discussed whether having errno as non-zero is a condition to be concerned with at all. In my view, whenever errno is set, it should be handled, then cleared, but I not sure that everyone follows this principle.
Our conclusion was to assume that setting of errno may not necessarily indicate an earlier problem, so it is probably better just to ignore the previous contents and zero it as you suggest. > 828-834: The code looks ok. Can you add a comment indication what will > be the command issued in the case of logical partition? These lines do not exist in auto_parse.c. What code are you referring to? > > usr/src/lib/liborchestrator/disk_parts.c: > > 406: Do you want to check whether committed_disk_target and > committed_disk_target->dparts before using it? This is probably safer. Will do. > 440: Same comment as that of 406 Same. > 443: Do you disallow SUNIXOS even it is a Solaris partition (and not > Linux Swap)? What do you recommend? > > usr/src/lib/liborchestrator/orchestrator_api.h: > > 299, 183: Why can't you use Om_NUMPART in line 183? I can, since I moved OM_NUMPART there from orchestrator_private.h. Will change. Thank you, William > > - Sundar >
