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

Add support for multiple port forwarding parameters in QEMU startup script #2575

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

John15321
Copy link
Member

@John15321 John15321 commented Jan 10, 2025

Add support for multiple port forwarding parameters in QEMU startup script

This PR introduces a new feature to the flatcar_qemu_startup.sh script, allowing users to specify multiple port forwards using the -f option. This enhancement improves the flexibility and usability of the script by enabling users to forward multiple ports from the host to the guest VM.

Key Changes:

Added the -f option to specify port forwards in the format host_port:guest_port. Updated the script to process multiple -f options and construct the appropriate QEMU command line arguments. Modified the usage instructions to include the new -f option.

How to Use:

To use the new port forwarding feature, specify the -f option followed by the host_port:guest_port pairs. Multiple -f options can be used to forward multiple ports. For example:

./flatcar_production_qemu.sh -M 6144 -f 25565:25565 -f 25575:25575 -f 2223:2223 -i ./config.json -- -display curses

This command sets the VM memory to 6144 MB, forwards ports 25565, 25575, and 2223, and uses a specified Ignition config file.

Testing Done:

Verified that the script correctly processes multiple -f options and constructs the QEMU command line with the specified port forwards. Tested the script with various combinations of port forwards to ensure proper functionality. Example command used for testing:

./flatcar_production_qemu.sh -M 6144 -f 25565:25565 -f 25575:25575 -f 2223:2223 -i ./config.json -- -display curses

Confirmed that the VM starts with the specified port forwards and that the ports are accessible from the host.

  • Changelog entries added in the respective changelog/ directory (user-facing change, bug fix, security fix, update)
  • Inspected CI output for image differences: /boot and /usr size, packages, list files for any missing binaries, kernel modules, config files, kernel modules, etc.

@tormath1 tormath1 added the main label Jan 10, 2025
@John15321 John15321 changed the title [DRAFT] Add port forwarding as multiple use parameters Add support for multiple port forwarding parameters in QEMU startup script Jan 10, 2025
@John15321 John15321 marked this pull request as ready for review January 10, 2025 13:03
@John15321
Copy link
Member Author

One thing that Im not sure about is where I should add the mention about it to the changelog file? 🤔

@tormath1
Copy link
Contributor

One thing that Im not sure about is where I should add the mention about it to the changelog file? 🤔

Yes please. In the changelog/changes folder, using the past tense, e.g:

Added support ... (flatcar/scripts#2575)

Copy link

@John15321 John15321 requested a review from a team January 10, 2025 15:28
Comment on lines 213 to 215
for port in $PORT_FORWARDS; do
host_port=$(echo "$port" | cut -d':' -f1)
guest_port=$(echo "$port" | cut -d':' -f2)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the ${PORT_FORWARDS} notation (with brackets) for all variables (same goes for ${port})

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed. please check :)

@@ -84,6 +86,9 @@ while [ $# -ge 1 ]; do
-p|-ssh-port)
SSH_PORT="$2"
shift 2 ;;
-f|-forward-port)
PORT_FORWARDS="${PORT_FORWARDS} $2"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit-pick: what about using FORWARD_PORT as variable name (to stay consistent with the flag name)

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't really have the same meaning as "SSH port" though.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tormath1 @chewi What about it now?

build_library/qemu_template.sh Outdated Show resolved Hide resolved
John15321 and others added 2 commits January 10, 2025 16:43
…ng.md

Co-authored-by: Mathieu Tortuyaux <mathieu.tortuyaux@gmail.com>
Co-authored-by: Mathieu Tortuyaux <mathieu.tortuyaux@gmail.com>
@@ -84,6 +86,9 @@ while [ $# -ge 1 ]; do
-p|-ssh-port)
SSH_PORT="$2"
shift 2 ;;
-f|-forward-port)
PORT_FORWARDS="${PORT_FORWARDS} $2"
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't really have the same meaning as "SSH port" though.

Comment on lines 213 to 215
for port in $PORT_FORWARDS; do
host_port=$(echo "$port" | cut -d':' -f1)
guest_port=$(echo "$port" | cut -d':' -f2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not using shell built-ins makes me cry. 😛

Suggested change
for port in $PORT_FORWARDS; do
host_port=$(echo "$port" | cut -d':' -f1)
guest_port=$(echo "$port" | cut -d':' -f2)
for port in ${PORT_FORWARDS}; do
host_port=${port%:*}
guest_port=${port#*:}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@John15321 John15321 self-assigned this Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Testing / in Review
Development

Successfully merging this pull request may close these issues.

3 participants