[libvirt] [PATCH 2/3] qemuDomainGetPreservedMounts: Prune nested mount points

[John Ferlan]

On 06/12/2017 11:57 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
> 
> There can be nested mount points. For instance /dev/shm/blah can
> be a mount point and /dev/shm too. It doesn't make much sense to
> return the former path because callers preserve the latter (and
> with that the former too). Therefore prune nested mount points.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 23b92606e..accf05a6f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7533,7 +7533,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
>                               size_t *ndevPath)
>  {
>      char **paths = NULL, **mounts = NULL;
> -    size_t i, nmounts;
> +    size_t i, j, nmounts;
>  
>      if (virFileGetMountSubtree(PROC_MOUNTS, "/dev",
>                                 &mounts, &nmounts) < 0)
> @@ -7545,6 +7545,27 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
>          return 0;
>      }
>  
> +    /* There can be nested mount points. For instance
> +     * /dev/shm/blah can be a mount point and /dev/shm too. It
> +     * doesn't make much sense to return the former path because
> +     * caller preserves the latter (and with that the former
> +     * too). Therefore prune nested mount points.
> +     * NB mounts[0] is "/dev". Should we start the outer loop
> +     * from the beginning of the array all we'd be left with is
> +     * just the first element. Think about it.
> +     */
> +    for (i = 1; i < nmounts; i++) {
> +        for (j = i + 1; j < nmounts;) {
> +            if (STRPREFIX(mounts[j], mounts[i])) {
> +                VIR_DEBUG("Dropping path %s because of %s", mounts[j], mounts[i]);
> +                VIR_DELETE_ELEMENT(mounts, j, nmounts);
> +            } else {
> +                j++;
> +            }

Ewww

I prefer a :

   j = i + 1;
   while (j < nmounts) {
      if ()
         ...
      else
         j++;
   }

IDC either way, it's the empty last for condition that causes my eyes to
roll! Besides it just is ripe for someone coming along and moving that
j++ up into the for as a fix which we both know would be a bad idea.

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> +        }
> +    }
> +
> +
>      if (VIR_ALLOC_N(paths, nmounts) < 0)
>          goto error;
>  
>