Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vsock: rename options to --client/--server #205

Merged
merged 5 commits into from
Dec 11, 2024
Merged

Conversation

matttbe
Copy link
Collaborator

@matttbe matttbe commented Dec 9, 2024

The idea is to have other types of remote connections later on, e.g. SSH. See the discussion on #204.

So to support future types later on, the syntax is now:

vng --server|--client [console|ssh] [--port PORT] [--remote-cmd COMMAND]

The 'ssh' option is not available yet, but by default, 'console' will be used, so users can just do:

$ vng --server
$ vng --client  # in another terminal

A new group for the parser has been added to avoid confusions: --port and --remote-cmd are linked to --client / --server.

While at it, the code for the console client/server has been moved to dedicated functions, grouped together. It will also be easy to support a new type later on, simply by calling a different function.

Note that this should only be a refactoring, the behaviour, apart from different option name, should not be modified.

The variables names have been renamed too. 'vsock' is now only used in virtme(-ng)-init and in the socat command.

  • a few minor changes linked to these options.

The idea is to have other types of remote connections later on, e.g.
SSH.

So to support future types later on, the syntax is now:

  vng --server|--client [console|ssh] [--port PORT] [--remote-cmd COMMAND]

The 'ssh' option is not available yet, but by default, 'console' will be
used, so users can just do:

  $ vng --server
  $ vng --client  # in another terminal

A new group for the parser has been added to avoid confusions: --port
and --remote-cmd are linked to --client/--server.

While at it, the code for the console client/server has been moved to
dedicated functions, grouped together. It will also be easy to support a
new type later on, simply by calling a different function.

Note that this should only be a refactoring, the behaviour, apart from
different option name, should not be modified.

The variables names have been renamed too. 'vsock' is now only used in
virtme(-ng)-init and in the socat command.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
So it can also be used with SSH, and sounds more "familiar".

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
Exit with an error if they are both defined.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
Print the socat command instead of executing it.

It might be more useful for the ssh command later, but still good to
respect --dry-run there.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
For the --server and --client options.

Plus mention the host's kernel needs to support VSOCK.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
@matttbe matttbe requested a review from arighi December 9, 2024 18:43
matttbe added a commit to multipath-tcp/mptcp-upstream-virtme-docker that referenced this pull request Dec 9, 2024
The new names, see [1]

Link: arighi/virtme-ng#205 [1]
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
Copy link
Owner

@arighi arighi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. This is great, thank you so much for working at this @matttbe !!! I left a small comment but overall it looks great.

virtme/commands/run.py Show resolved Hide resolved
@matttbe
Copy link
Collaborator Author

matttbe commented Dec 11, 2024

Sorry for the late review.

Less than 48h, that's not a late review ;-)

@matttbe matttbe merged commit e499d36 into arighi:main Dec 11, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants