[augeas-devel] Re: Review of the Augeas API
Jim Meyering
jim at meyering.net
Mon Apr 21 16:02:38 UTC 2008
David Lutterkort <dlutter at redhat.com> wrote:
> Hi Jim,
>
> I have a favor to ask of you: I want to sanction the Augeas API[1] as
> stable and maintained indefinitely. The one change from the current
> state I plan to make is to merge the aug_get and aug_exists calls into
> one
>
> int aug_get(augeas_t aug, const char *path, const char **value)
>
> call where the return value is the same as what aug_exists returns
> currently, and, if VALUE is non-NULL, it is set to point to the value
> associated with PATH.
>
> I was just wondering if you could have a quick look at augeas.h and let
> me know if you think that there's anything in there that you think is
> problematic going forward.
>
> I'd really appreciate your input,
> David
>
> [1] It's very small - only about 10 calls defined in
> http://hg.et.redhat.com/misc/augeas?f=551c06f8b78d;file=src/augeas.h
Hi David,
I'll be happy to look.
On the Copyright line: add 2008
For the file-spanning #ifndef,
#ifndef __AUGEAS_H
I prefer to use a trailing "_".
Leading ones are reserved, technically. No big deal.
This is important: do not hide a pointer in a typedef:
typedef struct augeas *augeas_t;
You can even do it with just one name. I.e., do this instead:
(impinge less on user name-space)
typedef struct augeas augeas;
Even the *_t name-space is reserved, technically,
but only language lawyers care if you violate that one.
Besides, hiding the pointer in the type
makes it so the lack of "const" is neither
apparent nor easy to fix in cases like this:
const char *aug_get(augeas_t aug, const char *path);
Instead, that should be:
const char *aug_get(const struct augeas *aug, const char *path);
At least aug_exists, aug_match, aug_print should also
have a "const" first parameter. const-adding patch below.
At first I thought aug_save would be changed, too,
but see it modifies state of *AUG, so no.
aug_print can fail, so it must return an indication of success or failure.
Most of the above is pretty shallow. I haven't even really studied
how these functions are used or how the underlying data structures are
arranged, so please consider this a first pass.
One possibility for a slightly deeper suggestion: provide a "do_for_each"
style iterator function. Currently you can do that, but only by first
allocating/filling space for the list of all matches and then iterating
through that list. With a do_for_each function, you could do the same
job without the cost (space) of allocating that list.
------------------------
Not in the header, but I suggest you change aug_read_file to something
like libvirt's fread_file_lim function (to be added to gnulib when I
find time). Otherwise, reading a non-regular file will cause trouble
(like OOM, e.g., on some directory/systems, or unnecessary failure when
given a named pipe). Even reading a normal file can cause trouble if
it's too large relative to the size of available virtual memory, hence
that function's "max_len" parameter:
http://thread.gmane.org/gmane.comp.emulators.libvirt/5850/focus=5853
========================
changeset: 375:4199729c2795
tag: tip
user: Jim Meyering <meyering at redhat.com>
date: Mon Apr 21 17:43:11 2008 +0200
files: src/augeas.c src/augeas.h src/augtool.c
description:
Add "const" to API interfaces, where possible.
diff --git a/src/augeas.c b/src/augeas.c
--- a/src/augeas.c
+++ b/src/augeas.c
@@ -121,13 +121,13 @@
* SEGMENT = STRING ('[' N ']') ? | '*'
* where STRING is any string not containing '/' and N is a positive number
*/
-static struct path *make_path(struct tree *root, const char *path) {
+static struct path *make_path(const struct tree *root, const char *path) {
struct path *result;
CALLOC(result, 1);
if (result == NULL)
return NULL;
- result->root = root;
+ result->root = (struct tree *) root;
if (*path != SEP)
result->nsegments = 1;
@@ -531,7 +531,7 @@
return NULL;
}
-const char *aug_get(struct augeas *aug, const char *path) {
+const char *aug_get(const struct augeas *aug, const char *path) {
struct path *p = make_path(aug->tree, path);
const char *result = NULL;
int r;
@@ -589,7 +589,7 @@
return tree_set(aug->tree, path, value) == NULL ? -1 : 0;
}
-int aug_exists(struct augeas *aug, const char *path) {
+int aug_exists(const struct augeas *aug, const char *path) {
struct path *p = make_path(aug->tree, path);
int result;
@@ -751,7 +751,7 @@
return -1;
}
-int aug_match(struct augeas *aug, const char *pathin, char ***matches) {
+int aug_match(const struct augeas *aug, const char *pathin, char ***matches) {
struct path *p = NULL;
struct tree *tree;
int cnt = 0;
@@ -915,7 +915,7 @@
free_path(p);
}
-void aug_print(struct augeas *aug, FILE *out, const char *pathin) {
+void aug_print(const struct augeas *aug, FILE *out, const char *pathin) {
if (pathin == NULL || strlen(pathin) == 0) {
pathin = "/*";
}
@@ -932,7 +932,7 @@
free(aug);
}
-int tree_equal(struct tree *t1, struct tree *t2) {
+int tree_equal(const struct tree *t1, const struct tree *t2) {
while (t1 != NULL && t2 != NULL) {
if (!streqv(t1->label, t2->label))
return 0;
diff --git a/src/augeas.h b/src/augeas.h
--- a/src/augeas.h
+++ b/src/augeas.h
@@ -25,7 +25,7 @@
#ifndef __AUGEAS_H
#define __AUGEAS_H
-typedef struct augeas *augeas_t;
+typedef struct augeas augeas;
/* Flags to influence the behavior of Augeas. Pass a bitmask of these flags
* to AUG_INIT.
@@ -56,7 +56,7 @@
* Return a handle to the Augeas tree upon success. If initialization
* fails, returns NULL.
*/
-augeas_t aug_init(const char *root, const char *loadpath, unsigned int flags);
+augeas *aug_init(const char *root, const char *loadpath, unsigned int flags);
/* Lookup the value associated with PATH. Return NULL if PATH does not
* exist, or if it matches more than one node, or if that is the value
@@ -64,21 +64,21 @@
*
* See AUG_EXISTS on how to tell these cases apart.
*/
-const char *aug_get(augeas_t aug, const char *path);
+const char *aug_get(const struct augeas *aug, const char *path);
/* Set the value associated with PATH to VALUE. VALUE is copied into the
* internal data structure. Intermediate entries are created if they don't
* exist. Return 0 on success, -1 on error. It is an error if more than one
* node matches PATH.
*/
-int aug_set(augeas_t aug, const char *path, const char *value);
+int aug_set(augeas *aug, const char *path, const char *value);
/* Return 1 if there is exactly one node matching PATH, 0 if there is none,
* and -1 if there is more than one node matching PATH. You should only
* call AUG_GET for paths for which AUG_EXISTS returns 1, and AUG_SET for
* paths for which AUG_EXISTS returns 0 or 1.
*/
-int aug_exists(augeas_t aug, const char *path);
+int aug_exists(const augeas *aug, const char *path);
/* Create a new sibling LABEL for PATH by inserting into the tree just
* before PATH if BEFORE == 1 or just after PATH if BEFORE == 0.
@@ -89,12 +89,12 @@
*
* Return 0 on success, and -1 if the insertion fails.
*/
-int aug_insert(augeas_t aug, const char *path, const char *label, int before);
+int aug_insert(augeas *aug, const char *path, const char *label, int before);
/* Remove path and all its children. Returns the number of entries removed.
* All nodes that match PATH, and their descendants, are removed.
*/
-int aug_rm(augeas_t aug, const char *path);
+int aug_rm(augeas *aug, const char *path);
/* Return the number of matches of the path expression PATH in AUG. If
* MATCHES is non-NULL, an array with the returned number of elements will
@@ -120,7 +120,7 @@
* matches more than one path segment.
*
*/
-int aug_match(augeas_t aug, const char *path, char ***matches);
+int aug_match(const augeas *aug, const char *path, char ***matches);
/* Write all pending changes to disk. Return -1 if an error is encountered,
* 0 on success. Only files that had any changes made to them are written.
@@ -134,16 +134,16 @@
*
* If neither of these flags is set, overwrite the original file.
*/
-int aug_save(augeas_t aug);
+int aug_save(augeas *aug);
/* Print each node matching PATH and its descendants to OUT */
-void aug_print(augeas_t aug, FILE *out, const char *path);
+void aug_print(const augeas *aug, FILE *out, const char *path);
/* Close this Augeas instance and free any storage associated with
* it. After running AUG_CLOSE, AUG is invalid and can not be used for any
* more operations.
*/
-void aug_close(augeas_t aug);
+void aug_close(augeas *aug);
#endif
diff --git a/src/augtool.c b/src/augtool.c
--- a/src/augtool.c
+++ b/src/augtool.c
@@ -38,7 +38,7 @@
static struct command commands[];
-static augeas_t augeas = NULL;
+static augeas *augeas = NULL;
static const char *const progname = "augtool";
static unsigned int flags = AUG_NONE;
const char *root = NULL;
More information about the augeas-devel
mailing list