[libvirt] [PATCHv3 17/36] util: storagefile: Add canonicalization to virStorageFileSimplifyPath

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
> The recently introduced virStorageFileSimplifyPath is good at resolving
> relative portions of a path. To add full path canonicalization
> capability we need to be able to resolve symlinks in the path too.
> 
> This patch adds a callback to that function so that arbitrary storage
> systems can use this functionality.
> ---
>  src/libvirt_private.syms  |  1 +
>  src/util/virstoragefile.c | 82 +++++++++++++++++++++++++++++++++++++++++++++--
>  src/util/virstoragefile.h |  9 ++++++
>  tests/virstoragetest.c    | 80 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 170 insertions(+), 2 deletions(-)
> 

> 
> +static int
> +virStorageFileExplodePath(const char *path,
> +                          size_t at,
> +                          char ***components,
> +                          size_t *ncomponents)
> +{
> +    char **tmp = NULL;
> +    char **next;
> +    size_t ntmp = 0;
> +    int ret = -1;
> +
> +    if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp)))
> +        goto cleanup;
> +
> +    /* prepend */
> +    for (next = tmp; *next; next++) {
> +        if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0)

So this is the call that is not necessarily guaranteeing NULL termination...

> +            goto cleanup;
> +
> +        at++;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virStringFreeListCount(tmp, ntmp);

...and therefore why you wanted to add this function from 14/36.  Hmm,
aren't you just taking the split array and reversing its order?  Do you
really need VIR_INSERT_ELEMENT for each array element, or could you just
VIR_ALLOC_N an array already big enough and then do the assignment from
'n' to 'size - n' in a loop through all size entries, at which point
you'd be guaranteed a NULL terminator and fewer intermediate malloc calls?

> +    return ret;
> +}
> +
> +
>  char *
> -virStorageFileSimplifyPath(const char *path,
> -                           bool allow_relative)
> +virStorageFileSimplifyPathInternal(const char *path,
> +                                   bool allow_relative,
> +                                   virStorageFileSimplifyPathReadlinkCallback cb,
> +                                   void *cbdata)
>  {
>      bool beginSlash = false;
>      char **components = NULL;
>      size_t ncomponents = 0;
> +    char *linkpath = NULL;
> +    char *currentpath = NULL;
>      size_t i;
>      char *ret = NULL;
> 
> @@ -2012,6 +2046,40 @@ virStorageFileSimplifyPath(const char *path,
>              continue;
>          }
> 
> +        /* read link and stuff */
> +        if (cb) {
> +            int rc;
> +            if (!(currentpath = virStorageFileExportPath(components, i + 1,
> +                                                         beginSlash)))

My first thought when reading this in isolation was "Why are we changing
the environment variable PATH to export it"?  I'm not sure how much of
your existing series is impacted, but I'm wondering if using "name" is
better than "path" when referring to a file name (GNU Coding Standards
recommends this nomenclature for a reason, after all, even if glibc's
realpath() muddies the water by having a use of "path" for a non-PATH
entity).


> +                goto cleanup;
> +
> +            if ((rc = cb(currentpath, &linkpath, cbdata)) < 0)
> +                goto cleanup;
> +
> +            if (rc == 0) {
> +                if (linkpath[0] == '/') {
> +                    /* start from a clean slate */
> +                    virStringFreeListCount(components, ncomponents);
> +                    ncomponents = 0;
> +                    components = NULL;
> +                    beginSlash = true;
> +                    i = 0;
> +                } else {
> +                    VIR_FREE(components[i]);
> +                    VIR_DELETE_ELEMENT(components, i, ncomponents);
> +                }
> +
> +                if (virStorageFileExplodePath(linkpath, i, &components,
> +                                              &ncomponents) < 0)
> +                    goto cleanup;
> +
> +                VIR_FREE(linkpath);
> +                VIR_FREE(currentpath);
> +
> +                continue;
> +            }
> +        }
> +

I've run out of review time at the moment, I'll have to pick up here
again later...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140602/f6859dce/attachment-0001.sig>