[augeas-devel] [PATCH] Add aug_transform and the transform command. Add -t|--transform option to augtool

[David Lutterkort]

On Fri, 2012-08-03 at 00:05 +0200, Raphaël Pinson wrote:
> This is a rework of the previous patch which adds:
> 
>  - an aug_transform API call
>  - a transform command for aug_srun
>  - a -t|--transform option to augtool
> 
> ---
>  src/augeas.c           |   48 ++++++++++++++++++++++++
>  src/augeas.h           |   12 ++++++
>  src/augeas_sym.version |    1 +
>  src/augrun.c           |   33 ++++++++++++++++
>  src/augtool.c          |   98 ++++++++++++++++++++++++++++++++----------------
>  tests/run.tests        |   21 +++++++++++
>  tests/test-api.c       |   39 +++++++++++++++++++
>  7 files changed, 219 insertions(+), 33 deletions(-)

I very much like the idea of this; it might be bikeshedding, but I'd
feel the syntax '-t FILE:LENS' would be more intuitive.

Some more comments:

> diff --git a/src/augeas.c b/src/augeas.c
> index 22ebd14..998d27b 100644
> --- a/src/augeas.c
> +++ b/src/augeas.c
> @@ -1729,6 +1729,54 @@ int aug_to_xml(const struct augeas *aug, const char *pathin,
>      return -1;
>  }
>  
> +int aug_transform(struct augeas *aug, const char *lens, const char *file) {
> +    int r = 0;
> +    char *lenspath = malloc(strlen(lens)+14);
> +    int lenspathlen = sprintf(lenspath, "/augeas/load/%s", lens);

We never use malloc/calloc directly in Augeas; instead, we use the
macros from memory.h like ALLOC_N and REALLOC_N since they are much
safer.

In this case though, you should use

        r = xasprintf(&lenspath, "/augeas/load/%s", lens);
        ERR_NOMEM(r < 0, aug);
        
> +    char *inclpath = malloc(lenspathlen+16);
> +    sprintf(inclpath, "%s/incl[last()+1]", lenspath);
> +
> +    char *lenslenspath = malloc(lenspathlen+6);
> +    sprintf(lenslenspath, "%s/lens", lenspath);
> +
> +    char *lensname;
> +    if (strchr(lens, '.')) {
> +        lensname = malloc(strlen(lens)+1);
> +        strcpy(lensname, lens);
> +    } else {
> +        lensname = malloc(strlen(lens)+5);
> +        sprintf(lensname, "%s.lns", lens);
> +    }

Similarly, all these malloc + sprintf combinations should be replaced
with xasprintf (which is a safe wrapper around asprintf)

> +    api_entry(aug);

You need to call api_entry as the very first thing in a public API
function, before doing anything that could produce an error.

> +    ARG_CHECK(STREQ("", lens), aug, "aug_transform: LENS must not be empty");
> +    ARG_CHECK(STREQ("", file), aug, "aug_transform: FILE must not be empty");

Do these checks right after calling api_entry, before doing any
allocations.

> +    if (aug_match(aug, lenslenspath, NULL) == 0)
> +        r = aug_set(aug, lenslenspath, lensname);
> +        ERR_BAIL(aug);
> +
> +    r = aug_set(aug, inclpath, file);
> +    ERR_BAIL(aug);

Actually, you could replace a lot of this by using some of the internal
tree_* helper functions, like tree_child and tree_child_cr. For example:

        struct tree *xfm = tree_path_cr(aug->origin, 3, s_augeas, s_load, lens);
        struct tree *l = tree_child_cr(xfm, "lens");
        tree_set(l, lens);
        ...

Of course, there's lots of checking that none of those functions return
NULL that is missing.

> diff --git a/src/augtool.c b/src/augtool.c
> index ce4a8a3..0e202e7 100644
> --- a/src/augtool.c
> +++ b/src/augtool.c
> +static void add_transform(const char *t) {
> +    char delims[] = "=";
> +    char *lens, *file;
> +    char *transform = strdup(t);
> +    int r;
> +   
> +    lens = strtok(transform, delims);
> +    file = strtok(NULL, delims);

strtok is one of those functions that everybody loves to hate (have a
look at the BUGS section in the man page) While its use in augtool
should be fine, you could do the same with

        lens = transform;
        file = strchr(lens, '=');
        *file++ = '\0';
        
or for the syntax I'd prefer

        file = transform;
        lens = strrchr(file, ':');
        *lens++ = '\0';

again with lots of checks for NULL etc. missing.
 
> @@ -551,6 +577,12 @@ int main(int argc, char **argv) {
>              print_aug_error();
>          exit(EXIT_FAILURE);
>      }
> +    while ((t = argz_next(transforms, transformslen, t))) {
> +        add_transform(t);
> +	added_transform = true;
> +    }
>
> +    if (added_transform)
> +        r = aug_load(aug);

Stylistically, I'd do all this in add_transform (and rename it to
add_transforms)

> diff --git a/tests/test-api.c b/tests/test-api.c
> index 5726540..ef9cce5 100644
> --- a/tests/test-api.c
> +++ b/tests/test-api.c
> @@ -455,6 +455,45 @@ static void testToXml(CuTest *tc) {
>      aug_close(aug);
>  }
>  
> +static void testTransform(CuTest *tc) {

I am perfectly happy to do most of these tests in run.tests; the only
tests that need to go into test-api.c are ones where we need to test a
specific behavior of the API call, and can't do that from run.tests -
usually only tests that check specific error returns and the like,
though even most of those can be done from run.tests.

David