Glenn Lagasse wrote: > Hey Dave, > > * Dave Miner (dminer at opensolaris.org) wrote: >> Glenn Lagasse wrote: >>> I've got fixes for: >>> >>> 817 LiveCD should handle existing ZFS filesystems on target hard disk >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=817 >>> >>> and >>> >>> 818 iotracing needs some usability improvements >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=818 >>> >>> The webrev is located at: >>> >>> http://cr.opensolaris.org/~glagasse/817_818/ >>> >> Sorry for the delay in reviewing. >> >> Neither bug report explains why you created the new service to start I/O >> tracing. > > I'll add that information. > >> packages/utils/iotrace.desktop >> >> 5: let's put this /usr (like /usr/sbin) so it's not taking up ramdisk space >> (however little) > > Done. > >> packages/utils/live-io-tracing >> >> 32: let's let stderr go to the service log, rather than the console > > Ok, what are you propsing? Just removing the stderr redirect? Or do I > need to do more than that to get stderr to show up in the service log? >
Don't redirect, svc.startd sets stderr to the service log automatically. >> packages/utils/live-io-tracing.xml >> >> 35: you should always provide a comment explaining any service dependencies > > Done. > >> 47: this could just be a default instance rather than live-media, but I >> guess I don't care much. We use the live-media instance for the other >> services because they're replacing the default, which isn't an issue here. > > I'm open to suggestions. This is my first foray into smf so I basically > copied something I found which is probably not the most optimal example. > > At a minimum, is changing the instance name on line 47 from 'live-media' > to 'default' sufficient or is there more that can/should be done? > After thinking about this further, I remembered what I really thought should be done here was to move all of the filesystem mounting that's in live-devices-local out to a separate CD-only service and revert the CD to running the normal device/local:default instance. The mounting and I/O tracing should be separate services from each other as well, and have them both be :default instances. So, here you should change 47 to <create_default_instance enabled='true' /> The rest of what I just suggested above should be done separately, and not right now since it's a somewhat substantial change. Dave
