Please review this new webrev:
http://cr.opensolaris.org/~tsk/863.2/
I think I covered most of the suggestions by Danek and Dan. However I didn't
make any changes to libbe to handle changing the "list of dicts" to a "dict of
dicts" since that would have touched libbe libbe(python) and beadm. The risk
was to high at this point to change all that code. I will file a bug however.
Thanks
Tim
Tim Knitter wrote:
>
>
> Danek Duvall wrote:
>> On Thu, Apr 03, 2008 at 09:16:11PM -0600, Tim Knitter wrote:
>>
>>>> bootenv.py:
>>>>
>>>> - Please follow the indentation used everywhere else -- eight spaces.
>>> Yes I noticed this throughout the pkg stuff. Why spaces instead of
>>> tabs? spaces are tiring. I changed them to be consistent.
>>
>> Ask Stephen. I don't know what editor you use, but if you use vim:
>>
>> :set ai sw=8 et
>>
>> will give you the basics without a lot of pain. I'm not sure how to
>> coerce
>> /bin/vi to expand tabs, but there's little reason to use it at this
>> point,
>> anyway. Bart can probably give you the right settings for emacs.
>>
>>>> [ 70's druggie code fantasy elided ]
>>> Is this a requirement? I may not have time to implement this.
>>
>> I wouldn't be happy letting it into the gate as it is. I think it's
>> pretty
>> straightforward to implement, and I think pretty close to what Krister
>> had
>> asked you to do already, though he'd left more to the imagination.
>>
>
> OK I'll implement this before the end of the day tomorrow.
>
>>>> - line 30: are these constants defined in libbe, too? If so, I'd
>>>> extract
>>>> them from that, rather than having to keep two lists in sync.
>>>>
>>> Yes there currently defined like:
>>>
>>> #define BE_ATTR_MOUNTED "mounted"
>>>
>>> So I don't think there retrievable unless you know how to extract them?
>>
>> They should be properties on the module: libbe.be_name, and so on.
>
> yeah.
>
>>
>>>> - line 76: why is a "beList" a dictionary?
>>> Seemed the most appropriate type to use when creating the C wrapper
>>> and extracting values passed by be_list. So all BE keys and values
>>> are one dict entry and another BE's keys and values are another dict
>>> entry... Here's an example of the dict:
>>>
>>> [{'orig_be_name': 'BE3', 'space_used': 108032L, 'active': False,
>>> 'mountpoint': 'legacy', 'active_boot': False, 'mounted': False,
>>> 'orig_be_pool': 'rpool'}, {'status': False, 'mountpoint': 'legacy',
>>> 'mounted': False, 'space_used': 18432L, 'dataset': 'BE3/opt'},
>>> {'orig_be_name': 'BE1', 'space_used': 1924565504L, 'active': False,
>>> 'mountpoint': '/tt', 'active_boot': True, 'mounted': True,
>>> 'orig_be_pool': 'rpool'}, {'status': False, 'mountpoint': '/tt/opt',
>>> 'mounted': False, 'space_used': 18432L, 'dataset': 'BE1/opt'},
>>> {'date': 1207154668L, 'policy': 'static', 'snap_name': '[EMAIL PROTECTED]'}]
>>>
>>> So once I find BE3 I know I know all the other keys and values relate
>>> to that BE.
>>
>> So instead of a list of dicts, why not a dict of dicts, mapping a BE's
>> name
>> to its key/value dict? Though I'm a bit confused by your example, given
>
> Seemed a list of dicts made sense given what was returned by libbe:be_list.
>
>> the dicts have wildly differing keys.
>
> Those are BE dicts, dataset dicts and snapshot dicts.
>
>>
>> As with the class restructuring, I think these changes need to be made.
>> The loop you have is ugly and unnecessary.
>>
>
> The install gate is closed, I think, or the last I heard, so any changes
> in libbe won't be made by tomorrow.
>
>> Whether it has to get done for tomorrow, I dunno. I'll let Stephen tell
>> you that you get the time you need to make it right. I'd say make the
>> effort, but if it doesn't make it in before code freeze, then make sure
>> it's the first thing you work on afterwards.
>
> ok
>
>>
>>>> - I'm a bit confused by why you have the if clause on 91 at all.
>>>> If I'm
>>>> reading it correctly, if we run across the active BE, we end up
>>>> ignoring the fact that we were asked to create an object based on
>>>> "root". Is that correct? Why is that right?
>>> hmm. your right. if root = '/' then be_mountpoint = "legacy" so I
>>> needed to check be_active for the '/' case. However if we run across
>>> '/' before the intended target of the -R argument the this is wrong.
>>> Good catch!
>>
>> This is hardcoding some scarily broken zfs behavior. Please provide a
>> comment explaining that be_mountpoint will be "legacy" when the be in
>> question is the active BE mounted at /. I hope that the ZFS folks
>> correct
>> this. If they aren't going to, it's probably worthwhile having your code
>> go figure out what the real mountpoints are.
>
> Yeah I agree. The "legacy" notation will eventually go away so my
> changes need to be dynamic enough to deal with it now and later.
>
>>
>>>> - line 192: Don't we need to call bootadm for a non-live BE, too?
>>> The design was to only activate the live BE since it seemed less
>>> intrusive.
>>
>> I'm not talking about activation, but just calling bootadm. The bootadm
>> call isn't part of activation in the live BE case; why should it be in
>> the
>> non-live case?
>>
>
> I agree. I'll put it in.
>
> Thanks
> Tim
>
>> Danek
>
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
