[PATCH v2] Report file path and reason when aug_save fails

[Dominic Cleal]

When aug_save fails, the /augeas metadata tree is now searched to find a
possible reason for the failure and an error is raised containing the file
path, Augeas error code (pointing to the action that failed) and the message if
supplied (from strerror).

A failure to unlink a file now results in a message such as:

  error: unspecified error
  error: aug_save failed on /etc/sysconfig/network-scripts/ifcfg-em1:
  unlink_orig (Permission denied)

Multiple failures aren't reported in detail, but will be printed if debug is
enabled.
---
 src/drv_debian.c  | 21 ++++-----------------
 src/drv_redhat.c  | 21 ++++-----------------
 src/drv_suse.c    | 21 ++++-----------------
 src/dutil_linux.c | 40 ++++++++++++++++++++++++++++++++++++++++
 src/dutil_linux.h |  3 +++
 5 files changed, 55 insertions(+), 51 deletions(-)

diff --git a/src/drv_debian.c b/src/drv_debian.c
index 440b233..bc6e636 100644
--- a/src/drv_debian.c
+++ b/src/drv_debian.c
@@ -912,10 +912,6 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     struct netcf_if *result = NULL;
     xmlDocPtr ncf_xml = NULL, aug_xml = NULL;
     char *name = NULL;
-    int r;
-    struct augeas *aug = get_augeas(ncf);
-
-    ERR_BAIL(ncf);
 
     ncf_xml = parse_xml(ncf, xml_str);
     ERR_BAIL(ncf);
@@ -941,12 +937,8 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     bond_setup(ncf, name, true);
     ERR_BAIL(ncf);
 
-    r = aug_save(aug);
-    if (r < 0 && NCF_DEBUG(ncf)) {
-        fprintf(stderr, "Errors from aug_save:\n");
-        aug_print(aug, stderr, "/augeas//error");
-    }
-    ERR_THROW(r < 0, ncf, EOTHER, "aug_save failed");
+    if (aug_save_assert(ncf) < 0)
+        goto error;
 
     result = make_netcf_if(ncf, name);
     ERR_BAIL(ncf);
@@ -961,12 +953,7 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
 }
 
 int drv_undefine(struct netcf_if *nif) {
-    struct augeas *aug = NULL;
     struct netcf *ncf = nif->ncf;
-    int r;
-
-    aug = get_augeas(ncf);
-    ERR_BAIL(ncf);
 
     bond_setup(ncf, nif->name, false);
     ERR_BAIL(ncf);
@@ -974,8 +961,8 @@ int drv_undefine(struct netcf_if *nif) {
     rm_interface(ncf, nif->name);
     ERR_BAIL(ncf);
 
-    r = aug_save(aug);
-    ERR_COND_BAIL(r < 0, ncf, EOTHER);
+    if (aug_save_assert(ncf) < 0)
+        goto error;
 
     return 0;
  error:
diff --git a/src/drv_redhat.c b/src/drv_redhat.c
index 898601d..c72a03b 100644
--- a/src/drv_redhat.c
+++ b/src/drv_redhat.c
@@ -865,10 +865,6 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     xmlDocPtr ncf_xml = NULL, aug_xml = NULL;
     char *name = NULL;
     struct netcf_if *result = NULL;
-    int r;
-    struct augeas *aug = get_augeas(ncf);
-
-    ERR_BAIL(ncf);
 
     ncf_xml = parse_xml(ncf, xml_str);
     ERR_BAIL(ncf);
@@ -894,12 +890,8 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     bond_setup(ncf, name, true);
     ERR_BAIL(ncf);
 
-    r = aug_save(aug);
-    if (r < 0 && NCF_DEBUG(ncf)) {
-        fprintf(stderr, "Errors from aug_save:\n");
-        aug_print(aug, stderr, "/augeas//error");
-    }
-    ERR_THROW(r < 0, ncf, EOTHER, "aug_save failed");
+    if (aug_save_assert(ncf) < 0)
+        goto error;
 
     result = make_netcf_if(ncf, name);
     ERR_BAIL(ncf);
@@ -914,12 +906,7 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
 }
 
 int drv_undefine(struct netcf_if *nif) {
-    struct augeas *aug = NULL;
     struct netcf *ncf = nif->ncf;
-    int r;
-
-    aug = get_augeas(ncf);
-    ERR_BAIL(ncf);
 
     bond_setup(ncf, nif->name, false);
     ERR_BAIL(ncf);
@@ -927,8 +914,8 @@ int drv_undefine(struct netcf_if *nif) {
     rm_interface(ncf, nif->name);
     ERR_BAIL(ncf);
 
-    r = aug_save(aug);
-    ERR_COND_BAIL(r < 0, ncf, EOTHER);
+    if (aug_save_assert(ncf) < 0)
+        goto error;
 
     return 0;
  error:
diff --git a/src/drv_suse.c b/src/drv_suse.c
index b677c88..11e606c 100644
--- a/src/drv_suse.c
+++ b/src/drv_suse.c
@@ -1007,10 +1007,6 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     xmlDocPtr ncf_xml = NULL, aug_xml = NULL;
     char *name = NULL;
     struct netcf_if *result = NULL;
-    int r;
-    struct augeas *aug = get_augeas(ncf);
-
-    ERR_BAIL(ncf);
 
     ncf_xml = parse_xml(ncf, xml_str);
     ERR_BAIL(ncf);
@@ -1036,12 +1032,8 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     bond_setup(ncf, name, true);
     ERR_BAIL(ncf);
 
-    r = aug_save(aug);
-    if (r < 0 && NCF_DEBUG(ncf)) {
-        fprintf(stderr, "Errors from aug_save:\n");
-        aug_print(aug, stderr, "/augeas//error");
-    }
-    ERR_THROW(r < 0, ncf, EOTHER, "aug_save failed");
+    if (aug_save_assert(ncf) < 0)
+        goto error;
 
     result = make_netcf_if(ncf, name);
     ERR_BAIL(ncf);
@@ -1056,12 +1048,7 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
 }
 
 int drv_undefine(struct netcf_if *nif) {
-    struct augeas *aug = NULL;
     struct netcf *ncf = nif->ncf;
-    int r;
-
-    aug = get_augeas(ncf);
-    ERR_BAIL(ncf);
 
     bond_setup(ncf, nif->name, false);
     ERR_BAIL(ncf);
@@ -1069,8 +1056,8 @@ int drv_undefine(struct netcf_if *nif) {
     rm_interface(ncf, nif->name);
     ERR_BAIL(ncf);
 
-    r = aug_save(aug);
-    ERR_COND_BAIL(r < 0, ncf, EOTHER);
+    if (aug_save_assert(ncf) < 0)
+        goto error;
 
     return 0;
  error:
diff --git a/src/dutil_linux.c b/src/dutil_linux.c
index d2bbef4..651da2e 100644
--- a/src/dutil_linux.c
+++ b/src/dutil_linux.c
@@ -215,6 +215,46 @@ struct augeas *get_augeas(struct netcf *ncf) {
     return NULL;
 }
 
+int aug_save_assert(struct netcf *ncf) {
+    int r = -1;
+    const char *err, *errmsg, *path = "unknown";
+    struct augeas *aug = get_augeas(ncf);
+
+    ERR_BAIL(ncf);
+
+    r = aug_save(aug);
+    if (r >= 0)
+        goto done;
+
+    if (NCF_DEBUG(ncf)) {
+        fprintf(stderr, "Errors from aug_save:\n");
+        aug_print(aug, stderr, "/augeas//error");
+    }
+
+    if (aug_get(aug, "/augeas//error", &err) == 1) {
+        if (aug_get(aug, "/augeas//error/../path", &path) == 1) {
+            /* strip /files prefix */
+            if (path != NULL && *path != '\0')
+                path = strchrnul(path + 1, '/');
+        }
+        if (aug_get(aug, "/augeas//error/message", &errmsg) == 1) {
+            report_error(ncf, NETCF_EOTHER, "aug_save failed on %s: %s (%s)",
+                         path, err, errmsg);
+        } else {
+            report_error(ncf, NETCF_EOTHER, "aug_save failed on %s: %s",
+                         path, err);
+        }
+    } else if (aug_match(aug, "/augeas//error", NULL) > 1) {
+        report_error(ncf, NETCF_EOTHER, "aug_save failed: multiple failures");
+    } else {
+        report_error(ncf, NETCF_EOTHER, "aug_save failed: unknown failure");
+    }
+
+ error:
+ done:
+    return r;
+}
+
 ATTRIBUTE_FORMAT(printf, 4, 5)
 int defnode(struct netcf *ncf, const char *name, const char *value,
                    const char *format, ...) {
diff --git a/src/dutil_linux.h b/src/dutil_linux.h
index 64fe64f..f63cc5a 100644
--- a/src/dutil_linux.h
+++ b/src/dutil_linux.h
@@ -69,6 +69,9 @@ int remove_augeas_xfm_table(struct netcf *ncf,
 /* Get or create the augeas instance from NCF */
 struct augeas *get_augeas(struct netcf *ncf);
 
+/* Save changes in augeas and raise error with message on failure */
+int aug_save_assert(struct netcf *ncf);
+
 /* Define a node inside the augeas tree */
 ATTRIBUTE_FORMAT(printf, 4, 5)
 int defnode(struct netcf *ncf, const char *name, const char *value,
-- 
1.9.3

[Dominic Cleal]

On 05/09/14 09:21, Dominic Cleal wrote:
>  When aug_save fails, the /augeas metadata tree is now searched to
> find a
>  possible reason for the failure and an error is raised containing the file
>  path, Augeas error code (pointing to the action that failed) and the message if
>  supplied (from strerror).
>  
>  A failure to unlink a file now results in a message such as:
>  
>    error: unspecified error
>    error: aug_save failed on /etc/sysconfig/network-scripts/ifcfg-em1:
>    unlink_orig (Permission denied)
>  
>  Multiple failures aren't reported in detail, but will be printed if debug is
>  enabled.
>  ---
>   src/drv_debian.c  | 21 ++++-----------------
>   src/drv_redhat.c  | 21 ++++-----------------
>   src/drv_suse.c    | 21 ++++-----------------
>   src/dutil_linux.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   src/dutil_linux.h |  3 +++
>   5 files changed, 55 insertions(+), 51 deletions(-) 
This updated patch fixes some compilation errors and warnings in
drv_{debian,suse}.c that I'd missed.

-- 
Dominic Cleal
Red Hat Engineering

[Laine Stump]

On 09/05/2014 04:21 AM, Dominic Cleal wrote:
>  When aug_save fails, the /augeas metadata tree is now searched to
> find a
>  possible reason for the failure and an error is raised containing the file
>  path, Augeas error code (pointing to the action that failed) and the message if
>  supplied (from strerror).
>
>  A failure to unlink a file now results in a message such as:
>
>    error: unspecified error
>    error: aug_save failed on /etc/sysconfig/network-scripts/ifcfg-em1:
>    unlink_orig (Permission denied)
>
>  Multiple failures aren't reported in detail, but will be printed if debug is
>  enabled. 
Thanks for taking the time to do this! (and you even tested building on
debian and suse, which is greatly appreciated since I don't have systems
setup to do that).

The one change I made was to turn this:


>  +    if (aug_save_assert(ncf) < 0)
>  +        goto error; 
into this:


    aug_save_assert(ncf);
    ERR_BAIL(ncf);

just for consistency with the rest of the code (I didn't create the
ERR_*() macros, and have never been sure if I really liked them or not,
but consistency is nice, and that's the way everything in the netcf code
is written, so I've tried to keep it that way).

I *didn't* change the following to use ERR_COND_BAIL() though, because I
think the logic that is made obvious by the if() ... else ... constructs
would be obscured too much by the goto's that are implicit in
ERR_COND_BAIL():

>  +
>  +    if (aug_get(aug, "/augeas//error", &err) == 1) {
>  +        if (aug_get(aug, "/augeas//error/../path", &path) == 1) {
>  +            /* strip /files prefix */
>  +            if (path != NULL && *path != '\0')
>  +                path = strchrnul(path + 1, '/');
>  +        }
>  +        if (aug_get(aug, "/augeas//error/message", &errmsg) == 1) {
>  +            report_error(ncf, NETCF_EOTHER, "aug_save failed on %s: %s (%s)",
>  +                         path, err, errmsg);
>  +        } else {
>  +            report_error(ncf, NETCF_EOTHER, "aug_save failed on %s: %s",
>  +                         path, err);
>  +        }
>  +    } else if (aug_match(aug, "/augeas//error", NULL) > 1) {
>  +        report_error(ncf, NETCF_EOTHER, "aug_save failed: multiple
> failures");
>  +    } else {
>  +        report_error(ncf, NETCF_EOTHER, "aug_save failed: unknown failure");
>  +    }
>  +
>  + error:
>  + done:
>  +    return r;
>  +}
>  
I added you name to AUTHORS and pushed the result. Thanks again!

[Dominic Cleal]

On 08/09/14 16:51, Laine Stump wrote:
>  On 09/05/2014 04:21 AM, Dominic Cleal wrote:
> > When aug_save fails, the /augeas metadata tree is now searched to find a
> > possible reason for the failure and an error is raised containing the file
> > path, Augeas error code (pointing to the action that failed) and the message if
> > supplied (from strerror).
> >
> > A failure to unlink a file now results in a message such as:
> >
> >   error: unspecified error
> >   error: aug_save failed on /etc/sysconfig/network-scripts/ifcfg-em1:
> >   unlink_orig (Permission denied)
> >
> > Multiple failures aren't reported in detail, but will be printed if debug is
> > enabled.
>  
>  Thanks for taking the time to do this! (and you even tested building on
>  debian and suse, which is greatly appreciated since I don't have systems
>  setup to do that).
>  
>  The one change I made was to turn this:
>  
>  
> > +    if (aug_save_assert(ncf) < 0)
> > +        goto error;
>  
>  into this:
>  
>  
>      aug_save_assert(ncf);
>      ERR_BAIL(ncf);
>  
>  just for consistency with the rest of the code (I didn't create the
>  ERR_*() macros, and have never been sure if I really liked them or not,
>  but consistency is nice, and that's the way everything in the netcf code
>  is written, so I've tried to keep it that way). 
Thanks for the fixes, that makes a lot more sense.

Unsurprisingly, the style is very similar to core Augeas!

>  I *didn't* change the following to use ERR_COND_BAIL() though,
> because I
>  think the logic that is made obvious by the if() ... else ... constructs
>  would be obscured too much by the goto's that are implicit in
>  ERR_COND_BAIL(): 
Agreed.

Cheers,

-- 
Dominic Cleal
Red Hat Engineering