From 65ac65a6d2ac441050323ab919b8bcaf2b6335a6 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 28 Oct 2008 12:46:18 -0700 Subject: [PATCH] Improve the command interface so that it sends back acks or errors. --- include/nicira-ext.h | 14 ++++---- include/vconn.h | 1 + lib/vconn.c | 45 +++++++++++++++--------- secchan/executer.c | 84 +++++++++++++++++++++++++++++++------------- utilities/dpctl.c | 70 +++++++++++++++++++++--------------- 5 files changed, 139 insertions(+), 75 deletions(-) diff --git a/include/nicira-ext.h b/include/nicira-ext.h index 9a7a3d074..893b90dcd 100644 --- a/include/nicira-ext.h +++ b/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_SIGNALED = 0x4000, /* Exited due to signal. */ - NXT_STATUS_UNKNOWN = 0x2000, /* Exited for unknown reason. */ - NXT_STATUS_COREDUMP = 0x1000, /* Exited with core dump. */ - NXT_STATUS_EXITSTATUS = 0xff, /* Exit code mask if NXT_STATUS_EXITED. */ - NXT_STATUS_TERMSIG = 0xff, /* Signal number if NXT_STATUS_SIGNALED. */ + NXT_STATUS_EXITED = 1 << 31, /* Exited normally. */ + NXT_STATUS_SIGNALED = 1 << 30, /* Exited due to signal. */ + NXT_STATUS_UNKNOWN = 1 << 29, /* Exited for unknown reason. */ + NXT_STATUS_COREDUMP = 1 << 28, /* Exited with core dump. */ + NXT_STATUS_ERROR = 1 << 27, /* Command could not be executed. */ + 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. */ diff --git a/include/vconn.h b/include/vconn.h index 106e47acb..6057c8a34 100644 --- a/include/vconn.h +++ b/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 **); diff --git a/lib/vconn.c b/lib/vconn.c index 13e096a99..96b85e883 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -552,45 +552,56 @@ vconn_recv_block(struct vconn *vconn, struct ofpbuf **msgp) return retval; } -/* 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 - * returns a positive errno value, or EOF, and sets '*replyp' to null. +/* 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_transact(struct vconn *vconn, struct ofpbuf *request, - struct ofpbuf **replyp) +vconn_recv_xid(struct vconn *vconn, uint32_t xid, struct ofpbuf **replyp) { - uint32_t send_xid = ((struct ofp_header *) request->data)->xid; - int error; - - *replyp = NULL; - error = vconn_send_block(vconn, request); - if (error) { - ofpbuf_delete(request); - return error; - } 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 (send_xid == recv_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, send_xid); + "%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 + * returns a positive errno value, or EOF, and sets '*replyp' to null. + * + * 'request' is always destroyed, regardless of the return value. */ +int +vconn_transact(struct vconn *vconn, struct ofpbuf *request, + struct ofpbuf **replyp) +{ + uint32_t send_xid = ((struct ofp_header *) request->data)->xid; + int error; + + *replyp = NULL; + error = vconn_send_block(vconn, request); + ofpbuf_delete(request); + return error ? error : vconn_recv_xid(vconn, send_xid, replyp); +} + void vconn_wait(struct vconn *vconn, enum vconn_wait_type wait) { diff --git a/secchan/executer.c b/secchan/executer.c index 45f69c9df..b52d7f863 100644 --- a/secchan/executer.c +++ b/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) { - struct nx_command_reply *r; - struct ofpbuf *buffer; - - r = make_openflow_xid(sizeof *r, OFPT_VENDOR, child->xid, &buffer); - r->nxh.vendor = htonl(NX_VENDOR_ID); - 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 (WIFEXITED(status)) { + ofp_status = WEXITSTATUS(status) | NXT_STATUS_EXITED; + } else if (WIFSIGNALED(status)) { + ofp_status = WTERMSIG(status) | NXT_STATUS_SIGNALED; + } else { + ofp_status = NXT_STATUS_UNKNOWN; + } + 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. */ diff --git a/utilities/dpctl.c b/utilities/dpctl.c index bb4b87e49..4592a9a61 100644 --- a/utilities/dpctl.c +++ b/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"); - 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"); - } + run(vconn_send_block(vconn, request), "send"); - status = ntohl(ncr->status); - 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 { - fprintf(stderr, "process terminated for unknown reason"); - } - if (status & NXT_STATUS_COREDUMP) { - fprintf(stderr, " (core dumped)"); - } - putc('\n', stderr); + for (;;) { + struct ofpbuf *reply; + uint32_t status; - 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, -- 2.45.2