[PATCH 32/37] qemuSlirpStart: Simplify parameters
Jonathon Jongsma
jjongsma at redhat.com
Thu May 12 18:37:45 UTC 2022
On 5/10/22 10:20 AM, Peter Krempa wrote:
> The 'driver' can be taken from the private data of 'vm' and 'slirp' can
> be taken from private data of 'net', both of which we need anyways.
>
> Additionally by checking whether slirp needs to be started inside the
> function we don't need to do this logic in the callers.
>
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
> src/qemu/qemu_extdevice.c | 4 +---
> src/qemu/qemu_hotplug.c | 2 +-
> src/qemu/qemu_slirp.c | 10 +++++++---
> src/qemu/qemu_slirp.h | 4 +---
> 4 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index 537b130394..1e54d4ef2c 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -185,10 +185,8 @@ qemuExtDevicesStart(virQEMUDriver *driver,
>
> for (i = 0; i < def->nnets; i++) {
> virDomainNetDef *net = def->nets[i];
> - qemuSlirp *slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
>
> - if (slirp &&
> - qemuSlirpStart(slirp, vm, driver, net, incomingMigration) < 0)
> + if (qemuSlirpStart(vm, net, incomingMigration) < 0)
> return -1;
> }
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 8314d0e546..9eeba0210f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1314,7 +1314,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
> break;
>
> if (qemuSlirpOpen(slirp, driver, vm->def) < 0 ||
> - qemuSlirpStart(slirp, vm, driver, net, NULL) < 0) {
> + qemuSlirpStart(vm, net, NULL) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("Failed to start slirp"));
> goto cleanup;
> diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
> index c832cfc20b..e1f06573e3 100644
> --- a/src/qemu/qemu_slirp.c
> +++ b/src/qemu/qemu_slirp.c
> @@ -245,12 +245,13 @@ qemuSlirpSetupCgroup(qemuSlirp *slirp,
>
>
> int
> -qemuSlirpStart(qemuSlirp *slirp,
> - virDomainObj *vm,
> - virQEMUDriver *driver,
> +qemuSlirpStart(virDomainObj *vm,
> virDomainNetDef *net,
> bool incoming)
Personal taste, perhaps, but the name qemuSlirpStart() implies to me
that it is a function that acts on a qemuSlirp* object. With this
change, the qemuSlirp object might not even exist when the function is
called. I would personally prefer that the function be renamed to
reflect this fact. Something like qemuDomainStartSlirp() perhaps? Up to
you if you want to change anything.
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> {
> + qemuDomainObjPrivate *priv = vm->privateData;
> + virQEMUDriver *driver = priv->driver;
> + qemuSlirp *slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
> g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> g_autoptr(virCommand) cmd = NULL;
> g_autofree char *pidfile = NULL;
> @@ -262,6 +263,9 @@ qemuSlirpStart(qemuSlirp *slirp,
> VIR_AUTOCLOSE errfd = -1;
> bool killDBusDaemon = false;
>
> + if (!slirp)
> + return 0;
> +
> if (incoming &&
> !qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> diff --git a/src/qemu/qemu_slirp.h b/src/qemu/qemu_slirp.h
> index a9a09cd5f8..507ea720fa 100644
> --- a/src/qemu/qemu_slirp.h
> +++ b/src/qemu/qemu_slirp.h
> @@ -61,9 +61,7 @@ int qemuSlirpOpen(qemuSlirp *slirp,
> virQEMUDriver *driver,
> virDomainDef *def);
>
> -int qemuSlirpStart(qemuSlirp *slirp,
> - virDomainObj *vm,
> - virQEMUDriver *driver,
> +int qemuSlirpStart(virDomainObj *vm,
> virDomainNetDef *net,
> bool incoming);
>
More information about the libvir-list
mailing list