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.
>> - 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.
>> - 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
the dicts have wildly differing keys.
As with the class restructuring, I think these changes need to be made.
The loop you have is ugly and unnecessary.
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.
>> - 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.
>> - 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?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
