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

fix: Dynamically create netdev arguments to correctly include commas #2581

Merged

Conversation

John15321
Copy link
Member

@John15321 John15321 commented Jan 15, 2025

Fix excessive comma in -netdev argument

Fix the previous change, where an excessive comma was added to the -netdev argument when no port forwards were passed:

-netdev user,id=eth0,,hostfwd=tcp::2222-:22...

By dynamically creating the argument to correctly include commas.

How to use

$  ./flatcar_production_qemu.sh -M 6144 -f 25565:25565 -f 25575:25575 -f 2223:2223  -- -display curses

Testing done

Correctly starts up the machine and forwards the SSH port to 2222:

$ ./flatcar_production_qemu.sh -M 6144 -- -display curses

image

Correctly starts up the machine and forwards the SSH port and all of the mentioned ports in the argument:

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

image

  • 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.

@John15321 John15321 marked this pull request as ready for review January 15, 2025 13:35
@John15321 John15321 requested a review from a team January 15, 2025 13:35
@John15321 John15321 self-assigned this Jan 15, 2025
Copy link
Contributor

@chewi chewi left a comment

Choose a reason for hiding this comment

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

This is fine, but another way to do it is:

-netdev user,id=eth0${QEMU_FORWARDED_PORTS:+,}${QEMU_FORWARDED_PORTS},hostfwd=tcp::"${SSH_PORT}"-:22,hostname="${VM_NAME}"

Copy link
Contributor

@tormath1 tormath1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the quick fix. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be moved to bugfixes section.

@John15321
Copy link
Member Author

This is fine, but another way to do it is:

-netdev user,id=eth0${QEMU_FORWARDED_PORTS:+,}${QEMU_FORWARDED_PORTS},hostfwd=tcp::"${SSH_PORT}"-:22,hostname="${VM_NAME}"

I deleted this part:

# Construct the -netdev user argument dynamically
NETDEV_USER_ARGS="id=eth0,hostfwd=tcp::${SSH_PORT}-:22,hostname=${VM_NAME}"
if [ -n "${QEMU_FORWARDED_PORTS}" ]; then
    NETDEV_USER_ARGS="${NETDEV_USER_ARGS},${QEMU_FORWARDED_PORTS}"
fi

And replace the -netdev call with:

case "${VM_BOARD}" in
    amd64-usr)
        qemu-system-x86_64 \
            -name "$VM_NAME" \
            -m ${VM_MEMORY} \
            -netdev user,id=eth0${QEMU_FORWARDED_PORTS+,}${QEMU_FORWARDED_PORTS},hostfwd=tcp::"${SSH_PORT}"-:22,hostname="${VM_NAME}" \
            -device virtio-net-pci,netdev=eth0 \
            -object rng-random,filename=/dev/urandom,id=rng0 -device virtio-rng-pci,rng=rng0 \
            "$@"
        ;;
    arm64-usr)
        qemu-system-aarch64 \
            -name "$VM_NAME" \
            -m ${VM_MEMORY} \
            -netdev user,id=eth0${QEMU_FORWARDED_PORTS+,}${QEMU_FORWARDED_PORTS},hostfwd=tcp::"${SSH_PORT}"-:22,hostname="${VM_NAME}" \
            -device virtio-net-device,netdev=eth0 \
            -object rng-random,filename=/dev/urandom,id=rng0 -device virtio-rng-pci,rng=rng0 \
            "$@"
        ;;
    *) die "Unsupported arch" ;;
esac

Im not sure if Im doing something wrong here but Im getting double commas here:

./flatcar_production_qemu.sh -- -display curses
qemu-system-x86_64: -netdev user,id=eth0,,hostfwd=tcp::2222-:22,hostname=flatcar_production_qemu-4215-0-0-nightly-20250114-2100: Parameter 'id' expects an identifier
Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.

@chewi
Copy link
Contributor

chewi commented Jan 15, 2025

That's my fault. I edited my comment after a few moments to add a :. Without that, it still inserts the comma when the variable is set but blank.

Copy link

github-actions bot commented Jan 15, 2025

Build action triggered: https://github.com/flatcar/scripts/actions/runs/12827137845

@John15321
Copy link
Member Author

That's my fault. I edited my comment after a few moments to add a :. Without that, it still inserts the comma when the variable is set but blank.

Oh, got it, thanks! Now its working perfectly 😄

@John15321
Copy link
Member Author

Thanks for the tip Chewi! I updated the PR with his changes, and ran the same commands as in the PR test description and everything seems to be working 😄

@John15321 John15321 requested review from chewi and tormath1 January 15, 2025 14:35
Copy link
Contributor

@tormath1 tormath1 left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to rebase / squash your commits. Too bad we missed that at the review, maybe we should add a test case for those helpers.

This has to be backported on the alpha maintenance branch too.

docs: Add entrance to the changelog about the fix

Update changelog/changes/2025-01-15-qemu-startup-script-comma-fix.md

Co-authored-by: Mathieu Tortuyaux <[email protected]>
@John15321 John15321 force-pushed the fix-port-forwarding-qemustartscript branch from d649592 to 0f0fa2f Compare January 15, 2025 16:58
@John15321
Copy link
Member Author

LGTM. Feel free to rebase / squash your commits. Too bad we missed that at the review, maybe we should add a test case for those helpers.

This has to be backported on the alpha maintenance branch too.

Done, sorry I forgot to use your tip for squashing will try that next time😜

Also yeah, my fault I missed that during testing somehow. How would you guys go about creating a test for something like that?

@chewi
Copy link
Contributor

chewi commented Jan 15, 2025

I'm not sure where it would fit in our test infrastructure. I think it would be important to actually run QEMU so that you know whether the invocation is valid. Perhaps it could be started headless in the background and then terminated after a few seconds. You could then check whether the exit status was successful.

@chewi
Copy link
Contributor

chewi commented Jan 15, 2025

You wouldn't even need a real disk image because it doesn't matter whether it actually boots or not. You might need real firmware for it to start though.

@tormath1 tormath1 merged commit c2d18cf into flatcar:main Jan 17, 2025
1 check was waiting
@tormath1
Copy link
Contributor

Cherry-picked to:

  • flatcar-4214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants