Improve the command interface so that it sends back acks or errors.

include/nicira-ext.h

@@ -118,12 +118,14 @@ OFP_ASSERT(sizeof(struct nx_action_header) == 16);
 
 /* Status bits for NXT_COMMAND_REPLY. */
 enum {
-    NXT_STATUS_EXITED = 0x8000,   /* Exited normally. */
+    NXT_STATUS_EXITED = 1 << 31,   /* Exited normally. */
-    NXT_STATUS_SIGNALED = 0x4000, /* Exited due to signal. */
+    NXT_STATUS_SIGNALED = 1 << 30, /* Exited due to signal. */
-    NXT_STATUS_UNKNOWN = 0x2000,  /* Exited for unknown reason. */
+    NXT_STATUS_UNKNOWN = 1 << 29,  /* Exited for unknown reason. */
-    NXT_STATUS_COREDUMP = 0x1000, /* Exited with core dump. */
+    NXT_STATUS_COREDUMP = 1 << 28, /* Exited with core dump. */
-    NXT_STATUS_EXITSTATUS = 0xff, /* Exit code mask if NXT_STATUS_EXITED. */
+    NXT_STATUS_ERROR = 1 << 27,    /* Command could not be executed. */
-    NXT_STATUS_TERMSIG = 0xff,    /* Signal number if NXT_STATUS_SIGNALED. */
+    NXT_STATUS_STARTED = 1 << 26,  /* Command was started. */
+    NXT_STATUS_EXITSTATUS = 0xff,  /* Exit code mask if NXT_STATUS_EXITED. */
+    NXT_STATUS_TERMSIG = 0xff,     /* Signal number if NXT_STATUS_SIGNALED. */
 };
 
 /* NXT_COMMAND_REPLY. */

include/vconn.h

@@ -54,6 +54,7 @@ uint32_t vconn_get_ip(const struct vconn *);
 int vconn_connect(struct vconn *);
 int vconn_recv(struct vconn *, struct ofpbuf **);
 int vconn_send(struct vconn *, struct ofpbuf *);
+int vconn_recv_xid(struct vconn *, uint32_t xid, struct ofpbuf **);
 int vconn_transact(struct vconn *, struct ofpbuf *, struct ofpbuf **);
 
 int vconn_open_block(const char *name, int min_version, struct vconn **);

lib/vconn.c

@@ -552,6 +552,37 @@ vconn_recv_block(struct vconn *vconn, struct ofpbuf **msgp)
     return retval;
 }
 
+/* Waits until a message with a transaction ID matching 'xid' is recived on
+ * 'vconn'.  Returns 0 if successful, in which case the reply is stored in
+ * '*replyp' for the caller to examine and free.  Otherwise returns a positive
+ * errno value, or EOF, and sets '*replyp' to null.
+ *
+ * 'request' is always destroyed, regardless of the return value. */
+int
+vconn_recv_xid(struct vconn *vconn, uint32_t xid, struct ofpbuf **replyp)
+{
+    for (;;) {
+        uint32_t recv_xid;
+        struct ofpbuf *reply;
+        int error;
+
+        error = vconn_recv_block(vconn, &reply);
+        if (error) {
+            *replyp = NULL;
+            return error;
+        }
+        recv_xid = ((struct ofp_header *) reply->data)->xid;
+        if (xid == recv_xid) {
+            *replyp = reply;
+            return 0;
+        }
+
+        VLOG_DBG_RL(&rl, "%s: received reply with xid %08"PRIx32" != expected "
+                    "%08"PRIx32, vconn->name, recv_xid, xid);
+        ofpbuf_delete(reply);
+    }
+}
+
 /* Sends 'request' to 'vconn' and blocks until it receives a reply with a
  * matching transaction ID.  Returns 0 if successful, in which case the reply
  * is stored in '*replyp' for the caller to examine and free.  Otherwise
@@ -567,28 +598,8 @@ vconn_transact(struct vconn *vconn, struct ofpbuf *request,
 
     *replyp = NULL;
     error = vconn_send_block(vconn, request);
-    if (error) {
+    ofpbuf_delete(request);
-        ofpbuf_delete(request);
+    return error ? error : vconn_recv_xid(vconn, send_xid, replyp);
-        return error;
-    }
-    for (;;) {
-        uint32_t recv_xid;
-        struct ofpbuf *reply;
-
-        error = vconn_recv_block(vconn, &reply);
-        if (error) {
-            return error;
-        }
-        recv_xid = ((struct ofp_header *) reply->data)->xid;
-        if (send_xid == recv_xid) {
-            *replyp = reply;
-            return 0;
-        }
-
-        VLOG_DBG_RL(&rl, "%s: received reply with xid %08"PRIx32" != expected "
-                    "%08"PRIx32, vconn->name, recv_xid, send_xid);
-        ofpbuf_delete(reply);
-    }
 }
 
 void

secchan/executer.c

@@ -89,6 +89,11 @@ struct executer {
     int null_fd;                /* FD for /dev/null. */
 };
 
+static void send_child_status(struct relay *, uint32_t xid, uint32_t status,
+                              const void *data, size_t size);
+static void send_child_message(struct relay *, uint32_t xid, uint32_t status,
+                               const char *message);
+
 /* Returns true if 'cmd' is allowed by 'acl', which is a command-separated
  * access control list in the format described for --command-acl in
  * secchan(8). */
@@ -164,6 +169,8 @@ executer_remote_packet_cb(struct relay *r, void *e_)
     /* Verify limit on children not exceeded.
      * XXX should probably kill children when the connection drops? */
     if (e->n_children >= MAX_CHILDREN) {
+        send_child_message(r, request->header.xid, NXT_STATUS_ERROR,
+                           "too many child processes");
         VLOG_WARN("limit of %d child processes reached, dropping request",
                   MAX_CHILDREN);
         return false;
@@ -190,16 +197,22 @@ executer_remote_packet_cb(struct relay *r, void *e_)
 
     /* Check permissions. */
     if (!executer_is_permitted(e->s->command_acl, argv[0])) {
+        send_child_message(r, request->header.xid, NXT_STATUS_ERROR,
+                           "command not allowed");
         goto done;
     }
 
     /* Find the executable. */
     exec_file = xasprintf("%s/%s", e->s->command_dir, argv[0]);
     if (stat(exec_file, &s)) {
+        send_child_message(r, request->header.xid, NXT_STATUS_ERROR,
+                           "command not allowed");
         VLOG_WARN("failed to stat \"%s\": %s", exec_file, strerror(errno));
         goto done;
     }
     if (!S_ISREG(s.st_mode)) {
+        send_child_message(r, request->header.xid, NXT_STATUS_ERROR,
+                           "command not allowed");
         VLOG_WARN("\"%s\" is not a regular file", exec_file);
         goto done;
     }
@@ -207,6 +220,8 @@ executer_remote_packet_cb(struct relay *r, void *e_)
 
     /* Arrange to capture output. */
     if (pipe(output_fds)) {
+        send_child_message(r, request->header.xid, NXT_STATUS_ERROR,
+                           "internal error (pipe)");
         VLOG_WARN("pipe failed: %s", strerror(errno));
         goto done;
     }
@@ -237,6 +252,7 @@ executer_remote_packet_cb(struct relay *r, void *e_)
         /* Running in parent. */
         struct child *child;
 
+        send_child_status(r, request->header.xid, NXT_STATUS_STARTED, NULL, 0);
         VLOG_WARN("started \"%s\" subprocess", argv[0]);
         child = &e->children[e->n_children++];
         child->name = xstrdup(argv[0]);
@@ -249,6 +265,8 @@ executer_remote_packet_cb(struct relay *r, void *e_)
         set_nonblocking(output_fds[0]);
         close(output_fds[1]);
     } else {
+        send_child_message(r, request->header.xid, NXT_STATUS_ERROR,
+                           "internal error (fork)");
         VLOG_WARN("fork failed: %s", strerror(errno));
         close(output_fds[0]);
         close(output_fds[1]);
@@ -261,12 +279,42 @@ done:
     return true;
 }
 
+static void
+send_child_status(struct relay *relay, uint32_t xid, uint32_t status,
+                  const void *data, size_t size)
+{
+    if (relay) {
+        struct nx_command_reply *r;
+        struct ofpbuf *buffer;
+
+        r = make_openflow_xid(sizeof *r, OFPT_VENDOR, xid, &buffer);
+        r->nxh.vendor = htonl(NX_VENDOR_ID);
+        r->nxh.subtype = htonl(NXT_COMMAND_REPLY);
+        r->status = htonl(status);
+        ofpbuf_put(buffer, data, size);
+        update_openflow_length(buffer);
+        if (rconn_send(relay->halves[HALF_REMOTE].rconn, buffer, NULL)) {
+            ofpbuf_delete(buffer);
+        }
+    }
+}
+
+static void
+send_child_message(struct relay *relay, uint32_t xid, uint32_t status,
+                   const char *message)
+{
+    send_child_status(relay, xid, status, message, strlen(message));
+}
+
 /* 'child' died with 'status' as its return code.  Deal with it. */
 static void
 child_terminated(struct child *child, int status)
 {
+    struct ds ds;
+    uint32_t ofp_status;
+
     /* Log how it terminated. */
-    struct ds ds = DS_EMPTY_INITIALIZER;
+    ds_init(&ds);
     if (WIFEXITED(status)) {
         ds_put_format(&ds, "normally with status %d", WEXITSTATUS(status));
     } else if (WIFSIGNALED(status)) {
@@ -285,30 +333,18 @@ child_terminated(struct child *child, int status)
 
     /* Send a status message back to the controller that requested the
      * command. */
-    if (child->relay) {
+    if (WIFEXITED(status)) {
-        struct nx_command_reply *r;
+        ofp_status = WEXITSTATUS(status) | NXT_STATUS_EXITED;
-        struct ofpbuf *buffer;
+    } else if (WIFSIGNALED(status)) {
-
+        ofp_status = WTERMSIG(status) | NXT_STATUS_SIGNALED;
-        r = make_openflow_xid(sizeof *r, OFPT_VENDOR, child->xid, &buffer);
+    } else {
-        r->nxh.vendor = htonl(NX_VENDOR_ID);
+        ofp_status = NXT_STATUS_UNKNOWN;
-        r->nxh.subtype = htonl(NXT_COMMAND_REPLY);
-        if (WIFEXITED(status)) {
-            r->status = htonl(WEXITSTATUS(status) | NXT_STATUS_EXITED);
-        } else if (WIFSIGNALED(status)) {
-            r->status = htonl(WTERMSIG(status) | NXT_STATUS_SIGNALED);
-        } else {
-            r->status = htonl(NXT_STATUS_UNKNOWN);
-        }
-        if (WCOREDUMP(status)) {
-            r->status |= htonl(NXT_STATUS_COREDUMP);
-        }
-        ofpbuf_put(buffer, child->output, child->output_size);
-        update_openflow_length(buffer);
-        if (rconn_send(child->relay->halves[HALF_REMOTE].rconn, buffer,
-                       NULL)) {
-            ofpbuf_delete(buffer);
-        }
     }
+    if (WCOREDUMP(status)) {
+        ofp_status |= NXT_STATUS_COREDUMP;
+    }
+    send_child_status(child->relay, child->xid, ofp_status,
+                      child->output, child->output_size);
 }
 
 /* Read output from 'child' and append it to its output buffer. */

utilities/dpctl.c

@@ -1290,13 +1290,14 @@ static void
 do_execute(const struct settings *s, int argc, char *argv[])
 {
     struct vconn *vconn;
-    struct ofpbuf *request, *reply;
+    struct ofpbuf *request;
     struct nicira_header *nicira;
     struct nx_command_reply *ncr;
-    int status;
+    uint32_t xid;
     int i;
 
     nicira = make_openflow(sizeof *nicira, OFPT_VENDOR, &request);
+    xid = nicira->header.xid;
     nicira->vendor = htonl(NX_VENDOR_ID);
     nicira->subtype = htonl(NXT_COMMAND_REQUEST);
     ofpbuf_put(request, argv[2], strlen(argv[2]));
@@ -1307,34 +1308,47 @@ do_execute(const struct settings *s, int argc, char *argv[])
     update_openflow_length(request);
 
     open_vconn(argv[1], &vconn);
-    run(vconn_transact(vconn, request, &reply), "transact");
+    run(vconn_send_block(vconn, request), "send");
-    if (reply->size < sizeof *ncr) {
-        ofp_fatal(0, "reply is too short (%zu bytes < %zu bytes)",
-                  reply->size, sizeof *ncr);
-    }
-    ncr = reply->data;
-    if (ncr->nxh.header.type != OFPT_VENDOR
-        || ncr->nxh.vendor != htonl(NX_VENDOR_ID)
-        || ncr->nxh.subtype != htonl(NXT_COMMAND_REPLY)) {
-        ofp_fatal(0, "reply is invalid");
-    }
 
-    status = ntohl(ncr->status);
+    for (;;) {
-    if (status & NXT_STATUS_EXITED) {
+        struct ofpbuf *reply;
-        fprintf(stderr, "process terminated normally with exit code %d",
+        uint32_t status;
-                status & NXT_STATUS_EXITSTATUS);
-    } else if (status & NXT_STATUS_SIGNALED) {
-        fprintf(stderr, "process terminated by signal %d",
-                status & NXT_STATUS_TERMSIG);
-    } else {
-        fprintf(stderr, "process terminated for unknown reason");
-    }
-    if (status & NXT_STATUS_COREDUMP) {
-        fprintf(stderr, " (core dumped)");
-    }
-    putc('\n', stderr);
 
-    fwrite(ncr + 1, reply->size - sizeof *ncr, 1, stdout);
+        run(vconn_recv_xid(vconn, xid, &reply), "recv_xid");
+        if (reply->size < sizeof *ncr) {
+            ofp_fatal(0, "reply is too short (%zu bytes < %zu bytes)",
+                      reply->size, sizeof *ncr);
+        }
+        ncr = reply->data;
+        if (ncr->nxh.header.type != OFPT_VENDOR
+            || ncr->nxh.vendor != htonl(NX_VENDOR_ID)
+            || ncr->nxh.subtype != htonl(NXT_COMMAND_REPLY)) {
+            ofp_fatal(0, "reply is invalid");
+        }
+
+        status = ntohl(ncr->status);
+        if (status & NXT_STATUS_STARTED) {
+            /* Wait for a second reply. */
+            continue;
+        } else if (status & NXT_STATUS_EXITED) {
+            fprintf(stderr, "process terminated normally with exit code %d",
+                    status & NXT_STATUS_EXITSTATUS);
+        } else if (status & NXT_STATUS_SIGNALED) {
+            fprintf(stderr, "process terminated by signal %d",
+                    status & NXT_STATUS_TERMSIG);
+        } else if (status & NXT_STATUS_ERROR) {
+            fprintf(stderr, "error executing command");
+        } else {
+            fprintf(stderr, "process terminated for unknown reason");
+        }
+        if (status & NXT_STATUS_COREDUMP) {
+            fprintf(stderr, " (core dumped)");
+        }
+        putc('\n', stderr);
+
+        fwrite(ncr + 1, reply->size - sizeof *ncr, 1, stdout);
+        break;
+    }
 }
 
 static void do_help(const struct settings *s, int argc UNUSED,