[augeas-devel] [PATCH] Add aug_span API function
David Lutterkort
lutter at redhat.com
Wed Jan 26 01:19:57 UTC 2011
On Mon, 2011-01-24 at 23:03 -0500, Francis Giraldeau wrote:
> aug_span API function provides information about the node of the specified tree
> path. It sets the absolute path of the file name and indexes of label, value
> and span inside the node. The span includes all matching chars of the actual
> lens and it's descendant.
Nice.
> Hence, the top level node will span the entire file.
In my testing, 'span /files/etc/hosts' gave me an error. You might have
to special-case the toplevel node for a file and 'pull' the spans of its
children up.
> It returns 0 in case of success and -1 if the node is not associated to a file
> or if the path is invalid. The main use case is to make possible to display
> the related file with the node elements hightlighted in a UI.
On error, it would be useful to distinguish between 'node has no info'
and 'something else went wrong'. Add a AUG_ENOSPAN to aug_errcode_t in
augeas.h to indicate 'no node info'
> The API option AUG_NO_NODE_INDEX provided to aug_init disable the allocation of
> structures to keep node indexes.
The default should be reversed: since node info can use quite a bit of
memory, storing node info should default to off. Since it can also be
triggered by changing a special node in the tree, is it really useful to
also have a flag for this ?
Also, the code talks about 'index' a lot - I'd prefer if 'span' is used
throughout instead.
> ---
> .gnulib | 2 +-
This should not be touched by your patch. Looks like you need to do a
'git submodule update' or similar on your working tree.
> diff --git a/src/augeas.c b/src/augeas.c
> index 25ca016..3391530 100644
> --- a/src/augeas.c
> +++ b/src/augeas.c
> @@ -527,6 +535,16 @@ int aug_load(struct augeas *aug) {
> * (4) Remove entries from /augeas/files and /files that correspond
> * to directories without any files of interest
> */
> +
> + /* update flags according to option value */
> + if (aug_get(aug, AUGEAS_INDEX_OPTION, &option) == 1) {
> + if (strcmp(option, AUG_ENABLE) == 0) {
> + aug->flags &= ~AUG_NO_NODE_INDEX;
> + } else if (strcmp(option, AUG_DISABLE) == 0) {
> + aug->flags |= AUG_NO_NODE_INDEX;
> + }
> + }
What happens when AUGEAS_INDEX_OPTION is set to garbage ? Probably
better to require a specific value to enable
> @@ -841,6 +859,8 @@ struct tree *make_tree(char *label, char *value, struct tree *parent,
> tree_mark_dirty(tree);
> else
> tree->dirty = 1;
> + // FIXME: should allocation belong to make_tree?
> + //tree->node_info = make_node_info();
This should be removed.
> @@ -860,6 +880,7 @@ static void free_tree_node(struct tree *tree) {
> if (tree == NULL)
> return;
>
> + unref(tree->node_info, node_info);
Should struct node_info really be ref counted ? IOW, is there any way in
which one struct node_info will ever be used by more than one struct
tree ? Seems to me that a node_info only ever belongs to one struct
tree.
> @@ -943,6 +964,51 @@ int aug_rm(struct augeas *aug, const char *path) {
> return -1;
> }
>
> +int aug_span(struct augeas *aug, const char *path, char **filename,
> + uint *label_start, uint *label_end, uint *value_start, uint *value_end,
> + uint *span_start, uint *span_end) {
> + struct pathx *p = NULL;
> + int result;
> + struct tree *tree = NULL;
> +
> + api_entry(aug);
> + p = pathx_aug_parse(aug, aug->origin, path, true);
Needs an ERR_BAIL(aug) (in case path is an invalid path expression)
> + tree = pathx_first(p);
> + if (tree == NULL || tree->node_info == NULL){
> + result = -1;
> + goto error;
> + }
This should report more fine-grained errors; something along the lines
ERR_THROW(tree == NULL, aug, AUG_ENOMATCH, "No node matching %s", path);
ERR_THROW(tree->node_info == NULL, aug, AUG_ENOSPAN, "No span info for %s", path);
ERR_THROW(pathx_next(p) != NULL, aug, AUG_EMMATCH, "Multiple nodes match %s", path);
> + if (label_start != NULL)
> + *label_start = tree->node_info->label_start;
> + if (label_end != NULL)
> + *label_end = tree->node_info->label_end;
> + if (value_start != NULL)
> + *value_start = tree->node_info->value_start;
> + if (value_end != NULL)
> + *value_end = tree->node_info->value_end;
> + if (span_start != NULL)
> + *span_start = tree->node_info->span_start;
> + if (span_end != NULL)
> + *span_end = tree->node_info->span_end;
> +
> + /* We are safer here, make sure we have a filename */
> + if (tree->node_info->filename == NULL || tree->node_info->filename->str == NULL) {
> + *filename = strdup("");
> + } else {
> + *filename = strdup(tree->node_info->filename->str);
> + }
This needs to be enclosed in a 'if (*filename != NULL)' (or is passing
non-NULL *filename mandatory ?)
Also, you should do a AUG_ENOMEM(*filename == NULL, aug); the doc for
aug_span seems to guarantee to me that *filename will be non-NULL on
success.
> + result = 0;
> + ERR_BAIL(aug);
> + api_exit(aug);
> + return result;
> +
> + error:
> + api_exit(aug);
> + return result;
> +}
You need to free p both on success and on the error path. I'd write the
above as
result = 0;
error:
free_pathx(p);
api_exit(aug);
return result;
> diff --git a/src/augeas.h b/src/augeas.h
> index b4848cb..18beaba 100644
> --- a/src/augeas.h
> +++ b/src/augeas.h
> @@ -149,6 +150,23 @@ int aug_set(augeas *aug, const char *path, const char *value);
> */
> int aug_setm(augeas *aug, const char *base, const char *sub, const char *value);
>
> +/* Function: aug_span
> + *
> + * Get the information about the node associated with PATH. If the node is
> + * associated with a file, the filename, label and value start and end positions
> + * are set, and return value is 0. If the node associated with PATH doesn't
> + * belong to a file or is doesn't exists, filename and indexes are not set and
> + * return value is -1.
> + *
> + * Returns:
> + * 0 on success with filename, label_start, label_stop, value_start, value_end
> + * -1 on error
> + */
Should also mention that it is legal to pass NULL for any of these
params.
> +int aug_span(augeas *aug, const char *path, char **filename,
> + uint *label_start, uint *label_end, uint *value_start, uint *value_end,
> + uint *span_start, uint *span_end);
You can't use uint in augeas.h since it's not defined by any file that
augeas.h includes. Instead, you need to write out 'unsigned int'.
> diff --git a/src/augtool.c b/src/augtool.c
> index 7de8e9d..6208162 100644
> --- a/src/augtool.c
> +++ b/src/augtool.c
> @@ -661,6 +661,54 @@ static const struct command_def cmd_clearm_def = {
> "BASE will be modified."
> };
>
> +static void cmd_span(struct command *cmd) {
> + const char *path = arg_value(cmd, "path");
> + int r;
> + uint label_start, label_end, value_start, value_end, span_start, span_end;
> + char *filename;
> + const char *option;
> + // FIXME: add check to see if AUG_NO_NODE_INDEX is set
> +
> + if (aug_get(aug, AUGEAS_INDEX_OPTION, &option) != 1) {
> + printf("Error: option /augeas/index not found\n");
Instead: printf("Error: option " AUGEAS_INDEX_OPTION " not found\n");
> diff --git a/src/get.c b/src/get.c
> index dbab6fa..6c178c9 100644
> --- a/src/get.c
> +++ b/src/get.c
> @@ -661,16 +677,27 @@ static struct skel *parse_quant_maybe(struct lens *lens, struct state *state,
> static struct tree *get_subtree(struct lens *lens, struct state *state) {
> char *key = state->key;
> char *value = state->value;
> + struct node_info *node_info = state->node_info;
> +
> struct tree *tree = NULL, *children;
>
> state->key = NULL;
> state->value = NULL;
> + if (!(state->info->flags & AUG_NO_NODE_INDEX))
> + state->node_info = make_node_info(state->info);
> +
> children = get_lens(lens->child, state);
>
> tree = make_tree(state->key, state->value, NULL, children);
> + tree->node_info = state->node_info;
> +
> + if (state->node_info != NULL) {
> + update_span(node_info, state->node_info->span_start, state->node_info->span_end);
> + }
Why is this done here ? Isn't that something the caller of get_subtree
should do ?
> diff --git a/src/info.c b/src/info.c
> index 9363477..8052026 100644
> --- a/src/info.c
> +++ b/src/info.c
> @@ -113,6 +113,46 @@ void free_info(struct info *info) {
> free(info);
> }
>
> +struct node_info *make_node_info(struct info *info) {
> + struct node_info *node_info = NULL;
> + if (make_ref(node_info) < 0) {
> + unref(node_info, node_info);
You also need to return NULL here. In addition, anywhere where
make_node_info is called, the caller needs to check if NULL is returned
and bail out with an ENOMEM.
> diff --git a/src/info.h b/src/info.h
> index cc5d2ce..fe6a6bf 100644
> --- a/src/info.h
> +++ b/src/info.h
> @@ -51,6 +51,19 @@ struct info {
> uint16_t last_line;
> uint16_t last_column;
> ref_t ref;
> + int flags;
> +};
> +
> +struct node_info {
> + struct string *filename;
> + uint label_start;
> + uint label_end;
> + uint value_start;
> + uint value_end;
> + uint span_start;
> + uint span_end;
> + char is_first_update;
Making this a char doesn't help, since the ref_t afterwards needs to be
aligned anyway, i.e. there'll be a 3 byte hole. If you want to minimize
the size of struct node_info, you could drop is_first_update altogether
and indicate that the node_info hasn't been initialized yet be setting
span_start > span_end (e.g. span_start = 1, span_end = 0) since that
can't ever happen in legitimate use.
> diff --git a/src/internal.h b/src/internal.h
> index 71c5d42..57d39e8 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -90,6 +90,10 @@
> /* Where to put information about parsing of path expressions */
> #define AUGEAS_META_PATHX AUGEAS_META_TREE "/pathx"
>
> +/* Define: AUGEAS_INDEX_OPTION
> + * Enable or disable node indexes */
> +#define AUGEAS_INDEX_OPTION AUGEAS_META_TREE "/index"
As I said above, s/index/span/ everywhere. I find calling the API
function aug_span and everything else connected to it 'index' confusing.
> diff --git a/src/transform.c b/src/transform.c
> index 521414b..e068e51 100644
> --- a/src/transform.c
> +++ b/src/transform.c
> @@ -503,13 +503,13 @@ static int load_file(struct augeas *aug, struct lens *lens,
> struct info *info;
> make_ref(info);
> make_ref(info->filename);
> - info->filename->str = filename;
> + info->filename->str = strdup(filename);
Why the strdup here ? Won't that be leaked ?
> diff --git a/tests/test-api.c b/tests/test-api.c
> index 322f4a9..368da01 100644
> --- a/tests/test-api.c
> +++ b/tests/test-api.c
Nice work on the test.
David
More information about the augeas-devel
mailing list