[augeas-devel] aug_lens_get

[David Lutterkort]

On Tue, 2009-03-24 at 19:09 +0100, Jakub Hrozek wrote:
> The attached patch implements this via an aug_lens_get() function that
> is an addition to the public API.

Good stuff.

> The parsed string goes into (newly
> created) /augeas/text hierarchy, the node is specified as a parameter of
> aug_lens. 

This should not go under /augeas, but directly under /text in the
toplevel of the tree; /augeas should only contain metadata.

> The patch is not completely correct - I spent some time trying to get it
> right but figured that it would be faster to ask here..the thing is,
> that it crashes on subsequent invocations (tested from augtool) on the
> lns_get() call. 

Yeah, memory management is a little confusing; some parts (like the
tree) are managed directly, others are reference counted - any struct
that has a 'ref' field is reference counted.

The specific problem is the FREE(lens) in transform_string;
lens_from_name returns a 'borrowed' reference to a lens, i.e. does not
increment the ref count of the lens. Therefore, when you are done with
it, you should not do anything, neither free or unref it.

Also, generally, I try to have free_XXX functions for any struct;
calling free or FREE directly on an internal struct is almost always the
wrong thing to do.

Some more comments on the patch:

> diff --git a/src/augeas.c b/src/augeas.c
> index 60460ba..d22ae5d 100644
> --- a/src/augeas.c
> +++ b/src/augeas.c
> @@ -921,6 +921,35 @@ int aug_print(const struct augeas *aug, FILE *out, const char *pathin) {
>      return result;
> }
> 
> +int aug_lens_get(augeas *aug, const char *path, const char *lens,
> +                 const char *txt, size_t txt_len) {
> +    char *text = NULL;
> +    int result = -1;
> +
> +    /* append newline to the end of the string if needed */
> +    if(txt_len == 0 || txt[txt_len-1] != '\n') {

Small style nit: please put spaces after if, while, for etc., i.e. 'if
(cond)' not 'if(cond)'

> +        if(ALLOC_N(text, txt_len+2) < 0) {
> +            goto fini;
> +        }
> +        strcpy(text, txt);
> +        text[txt_len]   = '\n';
> +        text[txt_len+1] = '\0';
> +        txt_len += 2;
> +    } else {
> +        text = (char *) txt;
> +    }

Should we do that here automatically, too ? The reason I do that in
transform_load is a bit of a hack, but since all the lenses we have
expect that the file end with a newline, it's necessary.

When we get a string passed in, it might be more sensible to expect that
the caller makes sure the string is right. Ultimately, we should check
whether the lens requires a trailing '\n' and only add one as a last
resort - but that would require additional analysis on the lens.

> static int cmd_print(char *args[]) {
>      return aug_print(aug, stdout, cleanpath(args[0]));
> }
> @@ -389,6 +402,9 @@ static const struct command const commands[] = {
>        "Save all pending changes to disk. For now, files are not overwritten.\n"
>        "        Instead, new files with extension .augnew are created"
>      },
> +    { "lens_get", 3, 3, cmd_lens_get, "lens_get <PATH> <MODULE> <TEXT>",
> +      "Parse text according to MOFULE and store result at PATH/\n"
> +    },

The <MODULE> should be called <LENS>. The notation @Module is a
shorthand for the lens of the transform marked for autoload in Module,
and really only needed so that I can populate the /augeas/load hierarchy
on startup, since the lens for a transform might not have a name.

In the general case, I expect people will use the notation
'Module.lens', e.g. 'Hosts.lns', to refer to the lens 'lns' from the
Hosts module.

> diff --git a/src/internal.h b/src/internal.h
> index 407f184..2b7dbb6 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -59,6 +59,10 @@
>   * Information about files */
> #define AUGEAS_META_FILES AUGEAS_META_TREE AUGEAS_FILES_TREE
> 
> +/* Define: AUGEAS_TEXT_TREE
> + * A hierarchy where we keep directly parsed text */
> +#define AUGEAS_TEXT_TREE AUGEAS_META_TREE "/text"

Should just be "/text", i.e. leave the AUGEAS_META_TREE out.

Overall looks very good though.

David