Evan Layton wrote:
> Ethan Quach wrote:
>> Evan,
>>
>> Evan Layton wrote:
>>>
>>> I've made the following changes. A new webrev is available at
>>> http://cr.opensolaris.org/~evanl/9594v2
>>>
>>>>
>>>>
>>>> be_mount.c
>>>> ------------------
>>>> 332-343 - If we're really mounting root only, this chunk
>>>> needs to be at line 320 instead of here.
>>>
>>> That would cause us to skip mounting any of the BE's subordinate
>>> datasets. Isn't it possible that we could need those for getting
>>> things like zones information?
>>
>> Yes, if we mounted root only, we would have made an assumption
>> that what we're looking for must exist within the root dataset and
>> not in some subordinate dataset of the BE; though, the things we're
>> looking for are all in /etc, like /etc/vfstab and /etc/zones/
>> These things likely won't be in separate subordinate datasets,
>> but I'm not sure we can say that these are the only things we'll
>> ever look for going forward.
>
> Agreed, which is why I included the BE's root dataset and subordinate
> datasets. However I can also envision the possibility of maybe only
> wanting to mount the root dataset for the BE.
>
>>
>>> I can easily move this but I'm not
>>> sure this is the correct thing here. Maybe what we need is a flag
>>> for ROOT_ONLY as well as one for DS_ONLY (or NO_SHARED) that just
>>> mounts the BE and it's datasets?
>>
>> Given the above, it doesn't seem like a ROOT_ONLY flag is
>> what we need, but rather a NO_ZONES flag. And if we switch
>> it to this, it would be useful to expose it in the public libbe and
>> get rid of the private flag altogether.
>
> Why not both? A private one that just mounts the root dataset for the BE
Why implement this private root only one now if nothing's using
it right now?
> and the public one that mounts the BE and it's subordinate datasets
> (just skips all the shared FS's).
NO_SHARED seems kindof awkward since we already have
public flags to explicitly mount shared FS's. Wouldn't the absence
of those flags already imply no shared FS's? It seems to me
a NO_ZONES flag would make a bit more sense since that
achieves the mount scenario we're really trying to achieve and
does not cause any logically inconsistency with the existing public
flags.
>>>> 1060 - For cases where orig_mntpnt is non NULL, we
>>>> also need to cleanup and remove the temp directory
>>>> that the pool dataset was mounted on.
>>>
>>> fixed for this instance of the use of be_make_tmp_mountpoint()...
>>
>> I don't see the changes for this.
>
> hmm it should be right there at line 1050.
line 1050 of be_mount.c is a left curly bracket. What file
are you referring to?
>>>> be_utils.c
>>>> ---------------
>>>> 1301 - What is the reason why this is changed from
>>>> strcmp to strstr?
>>>
>>> changed back to strcmp
>>
>> You've got a strncmp there now instead, and this doesn't
>> look right either. If the line we've read in has a token named
>> "defaultfoo", this would match and it shouldn't. I think it
>> needs to be strcmp().
>
> If it has a line called defaultfoo grub won't know what to do with it
> either and will fail.
Sure, but that has nothing to do with our parsing logic here.
> However to be save I can put "default " as the string and check for that.
I'm not following what you're saying here. Aren't we already
using the string "default" to compare against? My point is that
this logic here (using strncmp) can give a false positive on a string
match when it doesn't match.
>>
>> Can you please also update usr/src/cmd/beadm/messages.py
>> with the new error codes, and the previous two that we
>> forgot to update from earlier fixes. (yeah, we've still got this
>> mess going on.)
>
> This ugly mess will be updated/fixed with the fixes for 9941 and not
> as part of this fix.
But can you add these to the enumeration in messages.py just for
now at least, for completeness of the way its currently implemented?
@@ -129,8 +129,12 @@
BE_ERR_NO_MOUNTED_ZONE,
BE_ERR_MOUNT_ZONEROOT,
BE_ERR_UMOUNT_ZONEROOT,
- BE_ERR_ZONES_UNMOUNT
- ) = range(4000, 4057)
+ BE_ERR_ZONES_UNMOUNT,
+ BE_ERR_FAULT,
+ BE_ERR_RENAME_ACTIVE,
+ BE_ERR_NO_MENU,
+ BE_ERR_DEV_BUSY
+ ) = range(4000, 4061)
# Error message dictionaries.
mBeadmErr = {}
thanks,
-ethan
