Ethan Quach wrote:
>
>
> 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?
>
Fair enough, I'll remove the private flags.
>> 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.
>
Valid point, I've changed it to NO_ZONES.
>>>>> 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.
>
In the lastest version it's line 1047 in be_unmount_pool() in
be_mount.c. I've added {} around it in the if statement so it's a bit
easier to see.
> 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.
>
I fixed this by having it check just past the end of "default"
if (strncmp(strtok(temp_line, BE_WHITE_SPACE),
"default", 8) == 0) {
>
>>>
>>> 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 = {}
>
>
done...
webrev http://cr.opensolaris.org/~evanl/9594v2/ is updated
>
> thanks,
> -ethan
>
