[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Restore error/cleanup logic for qemuBuildNetCommandLine



On Mon, Mar 14, 2016 at 07:28:12 -0400, John Ferlan wrote:
> The virDomainConfVMNWFilterTeardown was called in the error path of
> qemuBuildCommandLine once network setup was partially or fully completed
> using the last_good_net as the basis to determine which filters needed
> to be torn down.  Commit id 'ef2ab8fd' moved that inside the new
> qemuBuildNetCommandLine, so that lost the failure.  Moving that cleanup
> back outside the call to the more general failure case.
> 
> Signed-off-by: John Ferlan <jferlan redhat com>
> ---
> 
> Jan notes post push that the cleanup path after building the network
> command line was there for any other failures and not just the failure
> in building the network command line:
> 
> http://www.redhat.com/archives/libvir-list/2016-March/msg00561.html
> 
> Even though the teardown is called during qemuProcessStop from
> virDomainConfVMNWFilterTeardown, the cleanup here could still be done.
> 
> The reason for moving it inside was based on a different patch where the
> desire was to not leak anything that could have changed inside one of the
> moved qemuBuild*CommandLine functions. In this case, though since it's
> used more generally for cleanup, it seems a good idea to return it.
> 
> The other option would be to just let qemuProcessStop handle it and a
> different patch to remove the error reset logic.
> 
>  src/qemu/qemu_command.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ba8c216..2e15c05 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7659,10 +7659,10 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
>                          bool emitBootindex,
>                          size_t *nnicindexes,
>                          int **nicindexes,
> -                        int *bootHostdevNet)
> +                        int *bootHostdevNet,
> +                        int *last_good_net)

So ... this is fugly.

>  {
>      size_t i;
> -    int last_good_net = -1;
>  
>      if (!def->nnets) {
>          /* If we have -device, then we set -nodefault already */
> @@ -7697,9 +7697,9 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
>                                                qemuCaps, vlan, bootNet, vmop,
>                                                standalone, nnicindexes,
>                                                nicindexes) < 0)
> -                goto error;
> +                return -1;
>  
> -            last_good_net = i;
> +            *last_good_net = i;
>              /* if this interface is a type='hostdev' interface and we
>               * haven't yet added a "bootindex" parameter to an
>               * emulated network device, save the bootindex - hostdev
> @@ -7714,11 +7714,6 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
>          }
>      }
>      return 0;
> -
> - error:
> -    for (i = 0; last_good_net != -1 && i <= last_good_net; i++)
> -        virDomainConfNWFilterTeardown(def->nets[i]);

If you need to clean up a partial state, you should do it here and not
leak it in the caller ....

> -    return -1;
>  }
>  
>  
> @@ -8604,6 +8599,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>      bool emitBootindex = false;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      int bootHostdevNet = 0;
> +    int last_good_net = -1;
>  
>  
>      VIR_DEBUG("conn=%p driver=%p def=%p mon=%p json=%d "
> @@ -8725,7 +8721,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>  
>      if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone,
>                                  emitBootindex, nnicindexes, nicindexes,
> -                                &bootHostdevNet) < 0)
> +                                &bootHostdevNet, &last_good_net) < 0)
>          goto error;
>  
>      if (qemuBuildSmartcardCommandLine(logManager, cmd, def, qemuCaps) < 0)
> @@ -9280,6 +9276,8 @@ qemuBuildCommandLine(virConnectPtr conn,
>      /* free up any resources in the network driver
>       * but don't overwrite the original error */
>      originalError = virSaveLastError();
> +    for (i = 0; last_good_net != -1 && i <= last_good_net; i++)
> +        virDomainConfNWFilterTeardown(def->nets[i]);

While the caller should remove none of the filters or all, depending on
whether you've successfully created them or not. Leaking state is ugly
and prone to breakages.

>      virSetError(originalError);
>      virFreeError(originalError);
>      virCommandFree(cmd);
> -- 
> 2.5.0
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]