[augeas-devel] [PATCH] Add aug_span API function
Francis Giraldeau
francis.giraldeau at usherbrooke.ca
Sun Feb 6 20:03:39 UTC 2011
On Tue, 2011-01-25 at 17:19 -0800, David Lutterkort wrote:
> > 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.
Fixed.
> > 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'
Fixed.
>
> > 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
Fixed.
> Also, the code talks about 'index' a lot - I'd prefer if 'span' is used
> throughout instead.
Fixed. It should be clear now what relates to span. node_info, index and
other oddities are gone.
> > ---
> > .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.
This one slipped by mistake in the commit, but thanks, I just learned
how to update submodules! ;) Should we add the file .gnulib
to .gitignore to avoid the mistake? It's strange, because this file is
now a directory...
> > 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
Yeah, if set to garbage, then the option is not disabled. The right
value AUG_ENABLE is now required.
>
> > @@ -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.
I thought that it would make sens to allocate the span struct in
make_tree, since those two are associated, but the aug->flags is out of
scope there, and can't test a span must be allocated or not. But,
anyway, we want to allocate span only for nodes in the tree that are
backed by a file, so this is right.
> > @@ -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 ?
So right. Fixed.
>
> > @@ -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)
Fixed.
>
> > + 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);
That's beautiful. Fixed.
>
> > + 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 ?)
Passing NULL filename should not set the filename. Fixed.
>
> 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.
Fixed.
>
> > + 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;
Fixed.
>
> > 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.
Fixed.
>
> > +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'.
Fixed. I was not aware that uint was only defined in stdlib.h.
> > 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");
Fixed.
>
> > 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 ?
Since there is one span per subtree, I thought that it was a logical
place to put the related code into get_subtree. The only caller is
get_lens, hence putting that in the caller would be pretty much
equivalent.
>
> > 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.
Ok. I added error gotos in those functions. Could it be possible that
such error handling is not standardized in the code? Also, is there a
way to test this error handling code? If we limit memory of the process,
then we can't guarantee it will hit the memory limit at an exact place
in the code... and the process will be killed anyway. Handling ENOMEM
seems hopeless...
>
> > 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.
Fixed. Removed the ref count and the is_first_update flag, so the struct
is pretty minimal now, and the special case handling is done with test
of value UINT_MAX on span_start.
> > 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.
Done.
> > 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 ?
The caller free filename, then we need to make a copy. It doesn't leak
because the filename is unreferenced within free_info. I verified with
valgrind to be sure.
>
> > 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.
Thanks, I try to write the tests first ;)
And thanks for this amazing code review. I really want to avoid to
repeat those errors in future, so I added few info on the wiki about
coding standard, testing and other tips.
http://augeas.net/page/Code_maintenance
I will send on the ML the version 2 of the patch with the modifications
log.
Cheer,
Francis
More information about the augeas-devel
mailing list