OK, thanks, Dave.
On 20/05/2011 15:38, Dave Miner wrote: > Darren, I'm fine with your responses below. The error messaging > suggestions in target_selection.py were about style (explaining > corrective actions, not just what's wrong) since we don't have much user > interface other than the manifest comments and documentation. That's > something I would like to address as we work on polish, but not an > immediate issue. > > Dave > > On 05/19/11 07:42 PM, Darren Kenny wrote: >> 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 > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
