Hi Dave, Thanks for the comments, more below...
On 19/05/2011 21:10, Dave Miner wrote: > Darren, a few follow-ups below: > > On 05/18/11 06:46 PM, Darren Kenny wrote: >> Hi, >> >> I think it's about time I got out another version of the code review for the >> CUD >> AI project. >> >> I've uploaded the webrev at: >> >> http://cr.opensolaris.org/~dkenny/cud_ai-to-slim-2/ >> >> And for anyone that's reviewed the code before, there is a diff: >> >> http://cr.opensolaris.org/~dkenny/cud_ai-to-slim-2-diffs/ >> > > auto_install.py, 394: You added this comment in response to my review; > as I realize time is short here I'd suggest we leave this as-is and file > a bug. Matt and I discussed this, and it's actually not possible because we need to parse the manifest before we can determine what types and how many transfer checkpoints to register. I don't think we could do this from within another checkpoint. So I've removed the comment. > > target_selection.py: > > A number of my comments look like they're being deferred (error messages > and such). Please file bugs for them. There are several bugs logged w.r.t. error messages consistency and localization, I looked back at your comments and I'm not sure what you're referring to here. There was one about 'creating Logical' but that has been fixed. > > 475,631: It's unclear why dataset options are not allowed if we have the > ability to create file systems and zvols. The issues don't seem any > different than for a new pool. We can't allow you to set the dataset options here since this is the default options as set on the pool, and inherited by filesystems/zvols. If you're preserving the pool you really shouldn't change this. Also, many of these options are only able to be set on zpool creation. It is still possible to set specific options for a new dataset using the options tag : <filsystem ....> <options>OPTIONS HERE</options> </filesystem> > > 1440: Appending the s0 seems sketchy here; it'll be true on root pools > that are whole disk, but not on non-root pools using whole disks (at > least as far as ZFS reports; is there a layer between you and ZFS that > is modifying this?) No libzfs, when queried for the devices, is returning them with s0 appended always, it's the zpool command that's stripping the s0... > > 2133: the changes here seem to result in adding a slice to non-root > pools, which seems unnecessary and undesirable I'll change the code slightly to ensure it's only for root pools we do it. Just for clarity, there is an assumption here, that is based off the current AI client, that if you specify a disk with just a partition, e.g.: <disk> <disk_name ctd="c0d0"/> <partition name="1" action="create" part_type="191"/> </disk> then the assumption is that this should be used as a device in the root pool. A similar assumption is made if you specify a disk on it's own, with no partitions or slices. So based on this, we will add a slice if the partition is in the root pool, or it has no in_zpool/in_vdev values set. Thanks, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
