feat(qemu): add _extra_qemu_args() hook

[Rahul-Sutariya]

feat(qemu): add _extra_qemu_args() hook

Add a single overridable hook _extra_qemu_args() to the Qemu class
that returns additional command-line flags to append after the base
devices. Defaults to [] so existing behavior is unchanged.

[lurtz]

@Rahul-Sutariya What a coincidence. I am also working on extending the QEMU code: #123

[lurtz]

Changes look ok to me, but I am not really a code owner of ITF, thus not approving.

We should have a discussion what features the QEMU plugin should have and how its extension points look like.

[Rahul-Sutariya]

Changes look ok to me, but I am not really a code owner of ITF, thus not approving.

We should have a discussion what features the QEMU plugin should have and how its extension points look like.

Hi @lurtz, thanks for the feedback.

This PR is focused on making the Qemu class and QemuProcess extensible so that downstream plugins can add custom QEMU devices (for example, ivshmem) through inheritance and dependency injection, instead of having to duplicate the command assembly logic.

Regarding the broader discussion about the QEMU plugin's features and extension points, I think that is a separate design topic and outside the scope of this PR. I'm probably not the right person to drive that discussion, so I'd prefer to keep this change focused on the extensibility improvements it introduces.

[lurtz]

approving because it looks like nobody else is looking at this PR. Might there be another refactoring of this code or is it final now?

[Rahul-Sutariya]

approving because it looks like nobody else is looking at this PR. Might there be another refactoring of this code or is it final now?

This is final one, Thanks for reveiw!

[ltekieli]

@lurtz I will review today.

[ltekieli]

This PR adds two extension points: _extra_qemu_args() on Qemu, and qemu=None injection on QemuProcess.

For the stated goal of allowing downstream plugins to customize QEMU command-line flags, the _extra_qemu_args() hook looks like the key API change. The additional QemuProcess(qemu=...) constructor parameter seems less necessary, because a downstream plugin can already subclass QemuProcess and replace the constructed QEMU instance after calling super().__init__():

class CustomQemuProcess(QemuProcess):
    def __init__(self, path_to_qemu_image, available_ram, available_cores, **kwargs):
        super().__init__(path_to_qemu_image, available_ram, available_cores, **kwargs)
        self._qemu = CustomQemu(
            path_to_qemu_image,
            available_ram,
            available_cores,
            network_adapters=self._network_adapters,
            port_forwarding=self._port_forwarding,
        )

Unless this PR has a concrete composition or testability use case for injecting a prebuilt QEMU object, I would keep the public surface smaller and drop the qemu= constructor change. That would leave the PR focused on the Qemu command-line extension hook, with clearer ownership of the QEMU process lifecycle.

[Rahul-Sutariya]

This PR adds two extension points: _extra_qemu_args() on Qemu, and qemu=None injection on QemuProcess.

For the stated goal of allowing downstream plugins to customize QEMU command-line flags, the _extra_qemu_args() hook looks like the key API change. The additional QemuProcess(qemu=...) constructor parameter seems less necessary, because a downstream plugin can already subclass QemuProcess and replace the constructed QEMU instance after calling super().__init__():

class CustomQemuProcess(QemuProcess):
    def __init__(self, path_to_qemu_image, available_ram, available_cores, **kwargs):
        super().__init__(path_to_qemu_image, available_ram, available_cores, **kwargs)
        self._qemu = CustomQemu(
            path_to_qemu_image,
            available_ram,
            available_cores,
            network_adapters=self._network_adapters,
            port_forwarding=self._port_forwarding,
        )

Unless this PR has a concrete composition or testability use case for injecting a prebuilt QEMU object, I would keep the public surface smaller and drop the qemu= constructor change. That would leave the PR focused on the Qemu command-line extension hook, with clearer ownership of the QEMU process lifecycle.

Agreed — dropped the [qemu] constructor change., Thanks!

[lurtz]

How does it then work, if the subclassed Qemu object cannot be injected?

[ltekieli]

How does it then work, if the subclassed Qemu object cannot be injected?

@lurtz can you elaborate?

[lurtz]

@ltekieli My assumption was the @Rahul-Sutariya intended to subclass the Qemu class. The child class provides an implementation of _extra_qemu_args(self) and thus need to be injected into QemuProcess.

With the removal of the optional constructor argument I fail to understand how the child class can be injected. However I am not a Python expert and there might be another trick you are about to explain to me.

[ltekieli]

How does it then work, if the subclassed Qemu object cannot be injected?

@lurtz can you elaborate?

check the code snippet, instead of injecting the Custom class is instantiated directly in the subclass:

class CustomQemuProcess(QemuProcess):
    def __init__(self, path_to_qemu_image, available_ram, available_cores, **kwargs):
        super().__init__(path_to_qemu_image, available_ram, available_cores, **kwargs)
        self._qemu = CustomQemu(
            path_to_qemu_image,
            available_ram,
            available_cores,
            network_adapters=self._network_adapters,
            port_forwarding=self._port_forwarding,
        )

[lurtz]

@ltekieli I understand now. Thank you, it should work