[augeas-devel] Merging Configuration Status

[David Lutterkort]

Hi Christos,

first off, my apologies for letting this linger for so long - I was on
vacation, and am still catching up with the backlog from that. But I
don't plan on taking vacation any time soon, so I'll make sure I respond
quicker ;)

I'll just comment on the patch for now, and respond to the larger merge
issue in a separate email.

First off, some stylistic comments: please do not use tabs to indent,
only spaces, and make sure there is no trailing whitespace.

More comments inline:
> >From 248ea90591198943c9bd2b7a273168db95e47951 Mon Sep 17 00:00:00 2001
> From: Christos Bountalis <c.bountalis at gmail.com>
> Date: Thu, 9 Jun 2011 17:47:29 +0300
> Subject: [PATCH] Added aug_find_lense function to find and return used for loading a
>  specific file
> 
> Aug_find_lense is a function that accepts a path of a file, and tries to
> find the lense that was used to load this file in Augeas. Returns either
> a string representing the lense if it is located succesfully either null
> if the lense could not be found, or the file is not loaded in augeas.

The proper word is 'lens' (without the 'e' at the end), i.e. aug_find_lens

> diff --git a/src/augeas.c b/src/augeas.c
> index 88d151f..559bd49 100644
> --- a/src/augeas.c
> +++ b/src/augeas.c
> @@ -617,6 +617,31 @@ static int find_one_node(struct pathx *p, struct tree **match) {
>      return -1;
>  }
>  
> +char* aug_find_lense(struct augeas *aug,const char *path){
> +	//creating a path to run with aug_match in order to find the lense used
> +	//so it can use the same lense to load the other n number paths ...
> +	char *startstr="/augeas/load/*[ '";
> +	char *endstr="/' =~ glob(incl) + regexp('/.*') ]/lens";
> +	char *tempstr=malloc((strlen(startstr)+strlen(endstr)+strlen(path)+2)*sizeof(char));

Please use ALLOC_N and check for errors to allocate memory; in this
case, you don't need to do that though, since you should never use
sprintf, always xasprintf:

> +	sprintf(tempstr,"%s%s%s",startstr,path,endstr);

This should be:

     char *tempstr = NULL;
     int r;
        r = xasprintf(&tempstr, "%s%s%s", startstr, path, endstr);
        ERR_NOMEM(r < 0, aug);
        
> +	//proceed with running the match path for the lense
> +	int cnt;
> +	char **matches;
> +	cnt=aug_match(aug,tempstr,&matches);

You can use Augeas' existing error detection and reporting mechanisms
here (see errcode.h and errcodes[] at the top of augeas.c): the below

> +    if (cnt != 1)   {
> +		printf("Error locating the lense for the specific path\n");
> +		free(tempstr);
> +		return NULL;
> +    }else{
> +		char *val;
> +        aug_get(aug, matches[0], &val);
> +        //remove initial @
> +        *val=*val++;
> +        free(tempstr);
> +        return val;
> +	}
> +}

should be replaced with

                ERR_THROW(cnt == 0, aug, AUG_ENOMATCH, "could not locate lens for %s", path);
                ERR_THROW(cnt > 1, aug, AUG_EMMATCH, "found %d lenses for %s", cnt, path);
                char *result = NULL; /* This needs to go at the very top of the function */
                result = aug_get(aug, matches[0], &result);
        error:
             free(tempstr);
             for (int i=0; i < cnt; i++)
        	free(matches[i]);
             free(matches)
             return result;
        }

You should not strip the '@' from lens names - there are two different
ways in which Augeas indicates a lens: '@Module', meaning 'the lens from
Module that is marked for autoload', and 'Module.Lens' meaning 'the lens
Lens from module Module'.

> diff --git a/src/augeas.h b/src/augeas.h
> index 813b7a6..af387ff 100644
> --- a/src/augeas.h
> +++ b/src/augeas.h
> @@ -192,6 +192,18 @@ int aug_insert(augeas *aug, const char *path, const char *label, int before);
>   */
>  int aug_rm(augeas *aug, const char *path);
>  
> +/* Function: aug_find_lense
> + *
> + * Finds the lense used for loading the file given as path
> + * to Augeas tree

You need to say whether path is a path in the file system, or in the
Augeas tree, i.e. whether you want '/etc/hosts' or '/files/etc/hosts'.

David