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
