erec: Review erec_print()

A new requirement to erec for the upcoming JSON support is printing
records with file input descriptors without open stream. The approach is
to treat 'name' field as file name, open it, extract the offending line
and close it again.

Further changes to libnftables input parsing routines though have shown
that the whole concept of file pointer reuse in erec is tedious and not
worth keeping:

* Closed files are to be supported as well, so there needs to be
  fallback code for opening the file anyway.

* When input descriptor is duplicated from parser state into an error
  record, the file pointer is copied as well. Therefore care has to be
  taken to not free the parser state before any error records have been
  printed. This is the only point where old and duplicated input
  descriptors are connected.

Therefore drop struct input_descriptor's 'fp' field and just always open
the file by name. This way also the old stream offset doesn't have to be
restored after reading.

While being at it, this patch fixes two other (potential) problems:

* If the offending line from input contains tabs, add them at the right
  position in the marker buffer as well to avoid misalignment.

* The input file may not be seekable (/dev/stdin for instance), so skip
  printing of offending line and markers if it couldn't be read
  properly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/nftables.h b/include/nftables.h
index 75134def9b8d44980bfe188dab227f8d685e54a4..5f2da8b090a37b288951f79cc5d860669c031483 100644 (file)
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -114,10 +114,7 @@ struct input_descriptor {
        struct location                 location;
        enum input_descriptor_types     type;
        const char                      *name;
-       union {
-               const char              *data;
-               FILE                    *fp;
-       };
+       const char                      *data;
        unsigned int                    lineno;
        unsigned int                    column;
        off_t                           token_offset;

diff --git a/src/erec.c b/src/erec.c
index 226c51f552267751d6b3161e19b030ff317ddcca..617c04ade178bb8d21e820eb8fc11c3d1a31c6ad 100644 (file)
--- a/src/erec.c
+++ b/src/erec.c
@@ -118,16 +118,12 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
 {
        const struct location *loc = erec->locations, *iloc;
        const struct input_descriptor *indesc = loc->indesc, *tmp;
-       const char *line = NULL; /* silence gcc */
+       const char *line = NULL;
        char buf[1024] = {};
        char *pbuf = NULL;
        unsigned int i, end;
+       FILE *f;
        int l;
-       off_t orig_offset = 0;
-       FILE *f = octx->error_fp;
-
-       if (!f)
-               return;
 
        switch (indesc->type) {
        case INDESC_BUFFER:
@@ -136,13 +132,16 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
                *strchrnul(line, '\n') = '\0';
                break;
        case INDESC_FILE:
-               orig_offset = ftell(indesc->fp);
-               if (orig_offset >= 0 &&
-                   !fseek(indesc->fp, loc->line_offset, SEEK_SET) &&
-                   fread(buf, 1, sizeof(buf) - 1, indesc->fp) > 0 &&
-                   !fseek(indesc->fp, orig_offset, SEEK_SET))
+               f = fopen(indesc->name, "r");
+               if (!f)
+                       break;
+
+               if (!fseek(f, loc->line_offset, SEEK_SET) &&
+                   fread(buf, 1, sizeof(buf) - 1, f) > 0) {
                        *strchrnul(buf, '\n') = '\0';
-               line = buf;
+                       line = buf;
+               }
+               fclose(f);
                break;
        case INDESC_INTERNAL:
        case INDESC_NETLINK:
@@ -151,6 +150,8 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
                BUG("invalid input descriptor type %u\n", indesc->type);
        }
 
+       f = octx->error_fp;
+
        if (indesc->type == INDESC_NETLINK) {
                fprintf(f, "%s: ", indesc->name);
                if (error_record_names[erec->type])
@@ -160,32 +161,34 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
                        loc = &erec->locations[l];
                        netlink_dump_expr(loc->nle, f, debug_mask);
                }
-               fprintf(f, "\n");
-       } else {
-               if (indesc->location.indesc != NULL) {
-                       const char *prefix = "In file included from";
-                       iloc = &indesc->location;
-                       for (tmp = iloc->indesc;
-                            tmp != NULL && tmp->type != INDESC_INTERNAL;
-                            tmp = iloc->indesc) {
-                               fprintf(f, "%s %s:%u:%u-%u:\n", prefix,
-                                       tmp->name,
-                                       iloc->first_line, iloc->first_column,
-                                       iloc->last_column);
-                               prefix = "                 from";
-                               iloc = &tmp->location;
-                       }
+               fprintf(f, "\n\n");
+               return;
+       }
+
+       if (indesc->location.indesc != NULL) {
+               const char *prefix = "In file included from";
+               iloc = &indesc->location;
+               for (tmp = iloc->indesc;
+                    tmp != NULL && tmp->type != INDESC_INTERNAL;
+                    tmp = iloc->indesc) {
+                       fprintf(f, "%s %s:%u:%u-%u:\n", prefix,
+                               tmp->name,
+                               iloc->first_line, iloc->first_column,
+                               iloc->last_column);
+                       prefix = "                 from";
+                       iloc = &tmp->location;
                }
-               if (indesc->name != NULL)
-                       fprintf(f, "%s:%u:%u-%u: ", indesc->name,
-                               loc->first_line, loc->first_column,
-                               loc->last_column);
-               if (error_record_names[erec->type])
-                       fprintf(f, "%s: ", error_record_names[erec->type]);
-               fprintf(f, "%s\n", erec->msg);
+       }
+       if (indesc->name != NULL)
+               fprintf(f, "%s:%u:%u-%u: ", indesc->name,
+                       loc->first_line, loc->first_column,
+                       loc->last_column);
+       if (error_record_names[erec->type])
+               fprintf(f, "%s: ", error_record_names[erec->type]);
+       fprintf(f, "%s\n", erec->msg);
 
-               if (indesc->type != INDESC_INTERNAL)
-                       fprintf(f, "%s\n", line);
+       if (line) {
+               fprintf(f, "%s\n", line);
 
                end = 0;
                for (l = erec->num_locations - 1; l >= 0; l--) {
@@ -194,6 +197,10 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
                }
                pbuf = xmalloc(end + 1);
                memset(pbuf, ' ', end + 1);
+               for (i = 0; i < end && line[i]; i++) {
+                       if (line[i] == '\t')
+                               pbuf[i] = '\t';
+               }
                for (l = erec->num_locations - 1; l >= 0; l--) {
                        loc = &erec->locations[l];
                        for (i = loc->first_column ? loc->first_column - 1 : 0;

diff --git a/src/scanner.l b/src/scanner.l
index d908a8fefc4f4e0059170ce8be60d48a9b0fe9be..ba60ec17c32a803b7bde33ba9d7f3374205fe2a9 100644 (file)
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -662,7 +662,6 @@ static struct error_record *scanner_push_file(void *scanner, const char *filenam
                state->indesc->location = *loc;
        state->indesc->type     = INDESC_FILE;
        state->indesc->name     = xstrdup(filename);
-       state->indesc->fp       = f;
        init_pos(state);
        return NULL;
 }
@@ -891,7 +890,6 @@ void scanner_destroy(void *scanner)
                if (inpdesc && inpdesc->name) {
                        xfree(inpdesc->name);
                        inpdesc->name = NULL;
-                       fclose(inpdesc->fp);
                }
                yypop_buffer_state(scanner);
        } while (state->indesc_idx--);