search

zerovm / zerovm-cli

ZeroVM command line utilities
Apache License 2.0
6 stars 7 forks source link

Rewrite of the zerovm execution code #31

Closed larsbutler closed 10 years ago

larsbutler commented 10 years ago

The previous implementation of this rewrite was broken in many ways, and thus was disabled/reverted.

This implementation should fix all of those issues, but since there were no baseline tests in place to very functionality, please test this thoroughly by hand to ensure that all use cases are working properly.

In the initial rewrite, many things didn't work properly. The fixes with this change include:

Sorry for the code bomb. Things got a little "bulldozer-y".

larsbutler commented 10 years ago

I see something minor I want to change... New revision coming shortly

pkit commented 10 years ago

Have couple of problems with this code:

I will create a functional tests for this I promise, because you're right it looks messy right now and featureset is quite vague.

mgeisler commented 10 years ago

@pkit How can you even review this :-) I would suggest restructuring the commits so that the layout changes in function signatures come first. Then add any missing documentation next, and finally add new parameters along with the corresponding documentation. That would make it super easy to verify that the safe changes (reformatting, documenting existing code) are really safe.

Maybe there is a couple of tests that could be added for the existing code too? These tests could then later be expanded to test the new functionality as it is introduced.

pkit commented 10 years ago

How can you even review this

5 year experience in troubleshooting code of unknown origins :)

Maybe there is a couple of tests that could be added for the existing code too?

That's what I meant by "I will write functional tests"

larsbutler commented 10 years ago

Okay, I'll look into these. Until then, consider this a work in progress. :)

On 15 Apr 2014, at 14:10, Constantine Peresypkin notifications@github.com wrote:

Have couple of problems with this code:

no environment variables handling? if std* channel is not a tty but a file, looks like something bad will happen... :) processed_cmd_args property and prepare_channels() function seem to use channel_ordinal inconsistently, or one of them is not used at all? no support for supplying default limits in config file (or any support for config files) I will create a functional tests for this I promise, because you're right it looks messy right now and featureset is quite vague.

— Reply to this email directly or view it on GitHub.

larsbutler commented 10 years ago

@pkit

  • no environment variables handling?

I missed the env vars. I'll add that.

  • if std* channel is not a tty but a file, looks like something bad will happen... :)

What sort of bad things will happen? :)

  • processed_cmd_args property and prepare_channels() function seem to use channel_ordinal - inconsistently, or one of them is not used at all?

Yeah, there's some similarity. I'll see if I can clean that up.

  • no support for supplying default limits in config file (or any support for config files)

I missed the limits config as well. I'll add that.

pkit commented 10 years ago

What sort of bad things will happen? :)

I think std* channels will not be created in manifest. :)

larsbutler commented 10 years ago

@pkit Ohhhhh, I see now.... the @ command line syntax isn't just used for single file channels; it's used for passing environment variables too! I had completely missed that. =)

mgeisler commented 10 years ago

@pkit

  • I missed the env vars. I'll add that.
  • What sort of bad things will happen? :)
  • Yeah, there's some similarity. I'll see if I can clean that up.
  • I missed the limits config as well. I'll add that.

Can we please get some inline comments? It's impossible to follow a discussion like this :)

pkit commented 10 years ago 
$ zvsh --zvm-image ~/python.tar python -c 'import os; print os.environ' @`echo TERM=$TERM`
{'TERM': 'xterm', 'UMASK': '42012'}
larsbutler commented 10 years ago

@mgeisler

Can we please get some inline comments? It's impossible to follow a discussion like this :)

You know you can edit other people's comments, right? =)

larsbutler commented 10 years ago

@pkit

I think std* channels will not be created in manifest. :)

You are correct; with this implementation, they would not be added to the manifest. So if sys.std{in,out,err} are not ttys, then what should happen? That part is not clear to me.

pkit commented 10 years ago

So if sys.std{in,out,err} are not ttys, then what should happen?

They should still be in manifest, but their [mapping] should be different. Usually std* can either be connected to char device or to file or to pipe. We have char, file and pipe mappings for that in [mapping]. I.e. I suppose zvsh needs to check out where the actual host std* device is connected and then alter the mapping.

mgeisler commented 10 years ago

Lars Butler notifications@github.com writes:

@mgeisler

Can we please get some inline comments? It's impossible to follow a discussion like this :)

You know you can edit other people's comments, right? =)

I don't like the idea of editing other people's comments. I'm also not sure that is a good way to communicate when people already have the old version in their inboxes.

larsbutler commented 10 years ago

@pkit

They should still be in manifest, but their [mapping] should be different. Usually std* can either be connected to char device or to file or to pipe. We have char, file and pipe mappings for that in [mapping]. I.e. I suppose zvsh needs to check out where the actual host std* device is connected and then > alter the mapping.

So what is the requirement then? Looking at the current master implementation, this is behavior I see:

Is this correct?

pkit commented 10 years ago

If std{in,out,err} is a tty, add channels to manifest and nvram [mapping] Else, add channels only to the manifest

Yep, the behavior is not complete. The default mapping for type=0 (sequential) channels: pipe Therefore I cover that one and tty one (pipe and char). To be on the safe side, I think zvsh should also check if host std* channel is file and set mapping to file then.

larsbutler commented 10 years ago

@pkit

Yep, the behavior is not complete. The default mapping for type=0 (sequential) channels: pipe Therefore I cover that one and tty one (pipe and char). To be on the safe side, I think zvsh should also check if host std* channel is file and set mapping to file then.

Okay, this helps, but I'm still not getting the full picture here. Here is the logic as I understand it so far. Can you help me fill in the blanks please?

Pseudo code:

# zerovm channel types: https://github.com/zerovm/zerovm/blob/master/doc/channels.txt#L19
if std{in,out,err} is a tty:
  include a channel definition in the manifest and nvram [mapping] section
  channel type for manifest is 0
  channel mode for nvram [mapping] is char
  # what else?
else:
  include a channels definition in the manifest only
  # what else?
pkit commented 10 years ago 
channel type for manifest is 0
if std{in,out,err} is a tty:
    include a channel definition in the manifest and nvram [mapping] section
    channel mode for nvram [mapping] is char
elif std{in,out,err} is a file:
    include a channel definition in the manifest and nvram [mapping] section
    channel mode for nvram [mapping] is file
else:
    include a channels definition in the manifest only
larsbutler commented 10 years ago

@pkit

Okay, in recent updates, I've fixed (I think) the following:

Please have a look, and let me know if you find anything else missing.

larsbutler commented 10 years ago

@pkit Okay, updated again. Does that look correct?

pkit commented 10 years ago

Will add tests, and then see if it's correct. :)

larsbutler commented 10 years ago

There isn't a compelling need to merge this right now. I'm going to close for now, and maybe reopen this later after some heavy rebasing and reorganizing.