ef: Improve parse error messages with context, caret, and field hints

src/ef-args.c

@@ -32,8 +32,13 @@ int argc_frame(int argc, const char *argv[], frame_t *f) {
         }
 
         if (!h) {
-            //po("ERROR: Invalid parameter: %s\n", argv[i]);
-            //return -1;
+            // Only overwrite if hdr_parse_fields didn't already set context
+            if (!PARSE_ERR_CTX.token) {
+                PARSE_ERR_CTX.token = argv[i];
+                PARSE_ERR_CTX.hdr_name = NULL;
+                PARSE_ERR_CTX.fields = NULL;
+                PARSE_ERR_CTX.fields_size = 0;
+            }
             return i;
         }
 
@@ -264,12 +269,83 @@ int argc_cmd(int argc, const char *argv[], cmd_t *c) {
     return i;
 }
 
+static int is_known_hdr(const char *name)
+{
+    int j;
+
+    for (j = 0; j < HDR_TMPL_SIZE; j++)
+        if (hdr_tmpls[j] && hdr_tmpls[j]->name &&
+            strcmp(name, hdr_tmpls[j]->name) == 0)
+            return 1;
+    return 0;
+}
+
+static void print_parse_error(int argc, const char *argv[], int err_idx)
+{
+    int j, col;
+    const char *tok = PARSE_ERR_CTX.token ? PARSE_ERR_CTX.token : argv[err_idx];
+
+    // Find the actual argv position of the error token.  PARSE_ERR_CTX.token
+    // is a pointer into a sub-array of argv, so pointer comparison works.
+    int tok_idx = err_idx;
+    if (PARSE_ERR_CTX.token) {
+        for (j = 0; j < argc; j++) {
+            if (argv[j] == PARSE_ERR_CTX.token) {
+                tok_idx = j;
+                break;
+            }
+        }
+    }
+
+    // Error headline
+    if (PARSE_ERR_CTX.hdr_name && !is_known_hdr(tok)) {
+        pe("error: '%s' is not a field of '%s' or a recognized header\n",
+           tok, PARSE_ERR_CTX.hdr_name);
+    } else if (PARSE_ERR_CTX.hdr_name) {
+        pe("error: '%s' is not a field of '%s'\n",
+           tok, PARSE_ERR_CTX.hdr_name);
+    } else if (PARSE_ERR_CTX.token) {
+        pe("error: '%s' is not a recognized header or command\n", tok);
+    } else {
+        pe("error: unexpected token '%s'\n", tok);
+    }
+
+    // Show the full command with a caret pointing to the bad token
+    pe("  ");
+    for (j = 0; j < argc; j++)
+        pe("%s ", argv[j]);
+    pe("\n");
+
+    // Compute column offset of the error token
+    col = 2; // leading "  "
+    for (j = 0; j < tok_idx; j++)
+        col += strlen(argv[j]) + 1;
+
+    pe("%*s", col, "");
+    for (j = 0; j < (int)strlen(tok); j++)
+        pe("^");
+    pe("\n");
+
+    // Hint: list available fields
+    if (PARSE_ERR_CTX.hdr_name && PARSE_ERR_CTX.fields) {
+        pe("  valid fields for '%s':", PARSE_ERR_CTX.hdr_name);
+        for (j = 0; j < PARSE_ERR_CTX.fields_size; j++) {
+            if (PARSE_ERR_CTX.fields[j].bit_width == 0)
+                continue;
+            pe(" %s", PARSE_ERR_CTX.fields[j].name);
+        }
+        pe("\n");
+    }
+}
+
 int argc_cmds(int argc, const char *argv[]) {
     struct timeval tv_now, tv_left, tv_begin, tv_end;
 
     int res, i = 0, cmd_idx = 0;
     cmd_t cmds[100] = {};
 
+    memset(&PARSE_ERR_CTX, 0, sizeof(PARSE_ERR_CTX));
+
     while (i < argc && cmd_idx < 100) {
         //po("%d, cmd[%d]\n", __LINE__, cmd_idx);
         res = argc_cmd(argc - i, argv + i, &cmds[cmd_idx]);
@@ -288,8 +364,8 @@ int argc_cmds(int argc, const char *argv[]) {
     }
 
     if (i != argc) {
-        po("Parse error! arg# %d out of %d, cmd_idx = %d\n", i, argc, cmd_idx);
-        return -1;
+        print_parse_error(argc, argv, i);
+        goto err;
     }
 
     capture_all_start();
@@ -318,9 +394,17 @@ int argc_cmds(int argc, const char *argv[]) {
     }
 
     return res;
+
+err:
+    for (i = 0; i < cmd_idx; ++i) {
+        cmd_destruct(&cmds[i]);
+    }
+
+    return -1;
 }
 
 int TIME_OUT_MS = 100;
+parse_err_ctx_t PARSE_ERR_CTX;
 
 int main_(int argc, const char *argv[]) {
     int opt;

src/ef.c

@@ -348,8 +348,13 @@ int hdr_parse_fields(frame_t *frame, struct hdr *hdr, int offset,
 
         f = find_field(hdr, argv[i]);
 
-        if (!f)
+        if (!f) {
+            PARSE_ERR_CTX.token = argv[i];
+            PARSE_ERR_CTX.hdr_name = hdr->name;
+            PARSE_ERR_CTX.fields = hdr->fields;
+            PARSE_ERR_CTX.fields_size = hdr->fields_size;
             return i;
+        }
 
         if (field_ignore) {
             field_ignore = 0;

src/ef.h

@@ -130,6 +130,15 @@ int field_copy(field_t *dst, const field_t *src);
 void field_destruct(field_t *f);
 GEN_ALLOC_CLONE_FREE(field);
 
+typedef struct {
+    const char *token;       // token that didn't match
+    const char *hdr_name;    // header being parsed (NULL if at header level)
+    int         fields_size; // number of fields in header
+    field_t    *fields;      // pointer to header's field array
+} parse_err_ctx_t;
+
+extern parse_err_ctx_t PARSE_ERR_CTX;
+
 typedef struct hdr {
     const char *name;
     const char *help;

test/parser-tests.rb

@@ -1,5 +1,27 @@
 #!/usr/bin/env ruby
 
+require 'open3'
+
+# Verify that invalid input produces the expected error on stderr and a
+# non-zero exit code.  +patterns+ is an array of strings that must all
+# appear in the stderr output (order-independent, substring match).
+def err cmd, *patterns
+    out, err, status = Open3.capture3("./ef #{cmd}")
+
+    if status.success?
+        raise "Command './ef hex #{cmd}' exited with 0, expected non-zero"
+    end
+
+    patterns.each do |pat|
+        unless err.include?(pat)
+            puts "STDERR: #{err}"
+            raise "Expected stderr to contain '#{pat}' for: ./ef hex #{cmd}"
+        end
+    end
+
+    puts "OK (err): #{cmd}"
+end
+
 def ok cmd, expect
     actual = %x[./ef hex #{cmd}]
     actual.chomp!
@@ -287,3 +309,30 @@ def ok_mask cmd, expect, expect_mask
 #ok("coap-opt num 4",
 #   "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000")
 
+# Parse error tests, verify helpful error messages on stderr
+# Typo in field name: "vud" is not a field of "ctag"
+err("hex eth smac 0xdead ctag vud 10 pcp 3",
+    "is not a field of 'ctag'",
+    "vud",
+    "valid fields for 'ctag'")
+
+# Unknown header after a valid header
+err("hex eth foo",
+    "is not a field of 'eth' or a recognized header",
+    "foo")
+
+# Completely unknown first token (at command level)
+err("hex bogus eth",
+    "bogus")
+
+# name command: error token is further in (caret must not point at "name")
+err("name f veth smac 0xdead",
+    "is not a recognized header or command",
+    "veth")
+
+# Typo in field name of a second header in the frame
+err("hex eth smac 1 ipv4 sop 1.2.3.4",
+    "is not a field of 'ipv4'",
+    "sop",
+    "valid fields for 'ipv4'")
+