William Schumann wrote: > 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. okay. >> 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? Sorry. It was in usr/src/lib/libict/ict.c
Thanks, Sundar >> >> 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? Target discovery finds out whether the partition of the type SUNIXOS is linux swap or not. If the partition is SUNIXOS and content is not Linux Swap, we should allow SUNIXOS. In the same file disk_parts.c, we check for Linux Swap in the function om_set_fdisk_target_attrs() >> >> 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 >>
