Alok Aggarwal wrote:
> Hi Keith,
>
> On Tue, 19 Jan 2010, Keith Mitchell wrote:
>
>> transfer_mod.py: I'm wondering why you chose to use a "break"
>> statement instead of updating either line 934 or line 941 such that
>> they're both either using "retry_timeout > 0" (or both using ">= 0").
>
> Neither of those work (I've tried them).
>
> If you start out with retry_timeout being '0' changing to
> "retry_timeout >= 0" doesn't work because you end up doing a retry
> when you shouldn't.
Move line 945 outside of the if block, and then you can do this
successfully. e.g.:
retry_timeout -= 10
if retry_timeout >= 0:
logsvc.write_log(TRANSFER_ID,
"Retrying in 10 seconds.\n")
time.sleep(10)
(But read on, because the more I think about it, the more I prefer the
use of "break")
> And, you end
> up setting retry_timeout to a negative number as well which results in
> an infinite loop.
I'm not sure I understand how a negative retry_timeout leads to an
infinite loop.
>
> Also if you use "retry_timeout > 0", then you don't
> execute the command even once if retry_timeout starts
> out being '0'.
>
> So, "break" seemed like the most straightforward
> and one that worked in my testing as well.
What's really confusing is now we've got multiple conditionals and some
of them are irrelevant, or seem irrelevant. For example, we check
retry_timeout in two places, and status == 1 in two places. In
particular, with your suggested "break", it seems like line 934 could
simply be converted to "while True:" (which would follow a common
paradigm for implementing "do-while" type syntax in Python, and seems
like a good idea). An additional 'break' would be needed for "if status
== 0", but I think it would make the entire code block much more legible.
>
>> Additionally, if, after the timeout gets to 0 we haven't successfully
>> executed this block, I think we'll want to explicitly raise a TAbort
>> with a reasonable message.
>
> I'm not sure I understand, can you rephrase that?
In other words, what happens if we break this loop due to the timeout?
It seems like this function exits successfully, and thus relies on some
function / finalizer script down the line to fail because this function
failed to set up the image area.
(This can probably be addressed in another bug if need be)
>
>> media-fs-root: It seems like we could leave the "sshd -c" and "svcadm
>> enable" as common code, and then simply check the variable again and
>> perform the set_lang/kbd commands in the liveCD case. (I just noticed
>> that sshd -c is not run for liveCD - I didn't realize you could ssh
>> in without running this command? Or does some magic happen elsewhere
>> that makes it work?)
>
> I'm merely reverting this code to what it used to be
> a couple of months back.
>
> The keys are not generated for the Live CD case for some
> reason. It could have something to do with making the Live CD
> vulnerable to attacks if you do this (since it's built with
> a known root password). OTOH, I don't see how ssh can be used without
> generating keys.
>
> Which leads me to question whether QE is even using this
> feature any more? AFAIK, the "enable ssh" feature was
> requested primarily by QE.
Thank you (and Dave) for the explanation.
>
> Thanks for the review!
You're welcome!
- Keith
> Alok
