Re: [PATCH 32/37] qemuSlirpStart: Simplify parameters

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>   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@xxxxxxxxxx>


>   {
> +    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);