You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mynewt.apache.org by Vipul Rahane <vi...@runtime.io> on 2017/03/02 19:33:23 UTC
Re: [2/2] incubator-mynewt-core git commit: MYNEWT-647 Changes to NMP over OIC scheme
+1, I like the simplification and the idea of NMP header being in one place. I had a few doubts which I clarified with Chris.
Regards,
Vipul Rahane
> On Mar 2, 2017, at 8:17 AM, ccollins@apache.org wrote:
>
> MYNEWT-647 Changes to NMP over OIC scheme
>
> This commit addresses the following three issues with NMP over OIC (OMP):
>
> 1. Parts of NMP header missing from OMP responses. This causes problems
> when multiplexing OMP among many target devices.
>
> 2. Parts of the NMP header are scattered in a few different places.
> This works, but it makes it difficult to debug packet traces.
>
> 3. NMP errors get lost in translation. Instead of an NMP message with
> an error code, the OMP server sends back a generic "Bad Request" OIC
> message.
>
> ***** Current Scheme
>
> *** Requests
>
> The NMP op is indicated by the OIC op:
> NMP read <=> OIC get
> NMP write <=> OIC put
>
> The NMP group and ID are indicated as CoAP URI Query options:
>
> gr=<group>
> id=<id>
>
> The remaining NMP header fields, seq and flags, are not present.
>
> The NMP payload is the entire CoAP request body. This is identical to the body
> of a plain NMP request.
>
> *** Responses
>
> The NMP header is not present. The NMP op is indicated in the payload (see
> below), but other header fields cannot be inferred.
>
> Payload consists of a single CBOR key-value pair. For read responses, the key
> is "r"; for write responses, the key is "w". The value is a second CBOR map
> containing the actual NMP response fields.
>
> Errors encountered during processing of NMP requests are reported via OIC error
> responses (bad request, internal server error).
>
> ***** Proposed Scheme
>
> *** Requests
>
> * The OIC op is always the same: put.
>
> * No URI Query CoAP options.
>
> * The NMP header is included in the payload as a key-value pair (key="_h").
> This pair is in the root map of the request and is a sibling of the other
> request fields. The value of this pair is the big-endian eight-byte NMP
> header with a length field of 0.
>
> *** Responses
>
> * As with requests, the NMP header is included in the payload as a
> key-value pair (key="_h").
>
> * No "r" or "w" field. The response fields are inserted into the root map
> as a sibling of the "_h" pair.
>
> * Errors encountered during processing of NMP requests are reported
> identically to other NMP responses (embedded NMP response).
>
> *** Notes
>
> * Keys that start with an underscore are reserved to the OIC manager
> protocol (e.g., "_h"). NMP requests and responses must not name any of
> their fields with a leading underscore.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/repo
> Commit: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/commit/860d2d26
> Tree: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/tree/860d2d26
> Diff: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/diff/860d2d26
>
> Branch: refs/heads/develop
> Commit: 860d2d266110918b4a6f923f1e18c9032d13c07d
> Parents: a4df93c
> Author: Christopher Collins <cc...@apache.org>
> Authored: Wed Mar 1 17:41:21 2017 -0800
> Committer: Christopher Collins <cc...@apache.org>
> Committed: Thu Mar 2 08:15:05 2017 -0800
>
> ----------------------------------------------------------------------
> mgmt/mgmt/include/mgmt/mgmt.h | 24 ++-
> mgmt/mgmt/src/mgmt.c | 22 ++-
> mgmt/newtmgr/include/newtmgr/newtmgr.h | 21 ---
> mgmt/oicmgr/src/oicmgr.c | 238 ++++++++++++++++++----------
> 4 files changed, 192 insertions(+), 113 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/mgmt/include/mgmt/mgmt.h
> ----------------------------------------------------------------------
> diff --git a/mgmt/mgmt/include/mgmt/mgmt.h b/mgmt/mgmt/include/mgmt/mgmt.h
> index 9b79723..3c402f2 100644
> --- a/mgmt/mgmt/include/mgmt/mgmt.h
> +++ b/mgmt/mgmt/include/mgmt/mgmt.h
> @@ -36,6 +36,10 @@ extern "C" {
> #define STR(x) #x
> #endif
>
> +#define NMGR_OP_READ (0)
> +#define NMGR_OP_READ_RSP (1)
> +#define NMGR_OP_WRITE (2)
> +#define NMGR_OP_WRITE_RSP (3)
>
> /* First 64 groups are reserved for system level newtmgr commands.
> * Per-user commands are then defined after group 64.
> @@ -63,6 +67,24 @@ extern "C" {
> #define MGMT_ERR_EBADSTATE (6) /* Current state disallows command. */
> #define MGMT_ERR_EPERUSER (256)
>
> +#define NMGR_HDR_SIZE (8)
> +
> +struct nmgr_hdr {
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> + uint8_t nh_op:3; /* NMGR_OP_XXX */
> + uint8_t _res1:5;
> +#endif
> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> + uint8_t _res1:5;
> + uint8_t nh_op:3; /* NMGR_OP_XXX */
> +#endif
> + uint8_t nh_flags; /* XXX reserved for future flags */
> + uint16_t nh_len; /* length of the payload */
> + uint16_t nh_group; /* NMGR_GROUP_XXX */
> + uint8_t nh_seq; /* sequence number */
> + uint8_t nh_id; /* message ID within group */
> +};
> +
> struct mgmt_cbuf;
>
> typedef int (*mgmt_handler_func_t)(struct mgmt_cbuf *);
> @@ -85,7 +107,7 @@ struct mgmt_group {
> sizeof(struct mgmt_handler));
>
> int mgmt_group_register(struct mgmt_group *group);
> -void mgmt_cbuf_setoerr(struct mgmt_cbuf *njb, int errcode);
> +int mgmt_cbuf_setoerr(struct mgmt_cbuf *njb, int errcode);
> const struct mgmt_handler *mgmt_find_handler(uint16_t group_id,
> uint16_t handler_id);
>
>
> http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/mgmt/src/mgmt.c
> ----------------------------------------------------------------------
> diff --git a/mgmt/mgmt/src/mgmt.c b/mgmt/mgmt/src/mgmt.c
> index 4ac232a..c1a2a1e 100644
> --- a/mgmt/mgmt/src/mgmt.c
> +++ b/mgmt/mgmt/src/mgmt.c
> @@ -137,14 +137,20 @@ err:
> return (NULL);
> }
>
> -void
> +int
> mgmt_cbuf_setoerr(struct mgmt_cbuf *cb, int errcode)
> {
> - CborEncoder *penc = &cb->encoder;
> - CborError g_err = CborNoError;
> - CborEncoder rsp;
> - g_err |= cbor_encoder_create_map(penc, &rsp, CborIndefiniteLength);
> - g_err |= cbor_encode_text_stringz(&rsp, "rc");
> - g_err |= cbor_encode_int(&rsp, errcode);
> - g_err |= cbor_encoder_close_container(penc, &rsp);
> + int rc;
> +
> + rc = cbor_encode_text_stringz(&cb->encoder, "rc");
> + if (rc != 0) {
> + return rc;
> + }
> +
> + rc = cbor_encode_int(&cb->encoder, errcode);
> + if (rc != 0) {
> + return rc;
> + }
> +
> + return 0;
> }
>
> http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/newtmgr/include/newtmgr/newtmgr.h
> ----------------------------------------------------------------------
> diff --git a/mgmt/newtmgr/include/newtmgr/newtmgr.h b/mgmt/newtmgr/include/newtmgr/newtmgr.h
> index 50deb15..1cf314d 100644
> --- a/mgmt/newtmgr/include/newtmgr/newtmgr.h
> +++ b/mgmt/newtmgr/include/newtmgr/newtmgr.h
> @@ -29,27 +29,6 @@
> extern "C" {
> #endif
>
> -#define NMGR_OP_READ (0)
> -#define NMGR_OP_READ_RSP (1)
> -#define NMGR_OP_WRITE (2)
> -#define NMGR_OP_WRITE_RSP (3)
> -
> -struct nmgr_hdr {
> -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> - uint8_t nh_op:3; /* NMGR_OP_XXX */
> - uint8_t _res1:5;
> -#endif
> -#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> - uint8_t _res1:5;
> - uint8_t nh_op:3; /* NMGR_OP_XXX */
> -#endif
> - uint8_t nh_flags; /* XXX reserved for future flags */
> - uint16_t nh_len; /* length of the payload */
> - uint16_t nh_group; /* NMGR_GROUP_XXX */
> - uint8_t nh_seq; /* sequence number */
> - uint8_t nh_id; /* message ID within group */
> -};
> -
> struct nmgr_transport;
>
> /**
>
> http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/860d2d26/mgmt/oicmgr/src/oicmgr.c
> ----------------------------------------------------------------------
> diff --git a/mgmt/oicmgr/src/oicmgr.c b/mgmt/oicmgr/src/oicmgr.c
> index d2110ef..d7d3591 100644
> --- a/mgmt/oicmgr/src/oicmgr.c
> +++ b/mgmt/oicmgr/src/oicmgr.c
> @@ -22,6 +22,7 @@
>
> #include <os/os.h>
> #include <os/endian.h>
> +#include <defs/error.h>
>
> #include <assert.h>
> #include <string.h>
> @@ -52,9 +53,6 @@ static struct omgr_state omgr_state = {
> .os_event.ev_cb = omgr_event_start,
> };
>
> -static void omgr_oic_get(oc_request_t *request, oc_interface_mask_t interface);
> -static void omgr_oic_put(oc_request_t *request, oc_interface_mask_t interface);
> -
> struct os_eventq *
> mgmt_evq_get(void)
> {
> @@ -67,124 +65,199 @@ mgmt_evq_set(struct os_eventq *evq)
> os_eventq_designate(&omgr_state.os_evq, evq, &omgr_state.os_event);
> }
>
> -static const struct mgmt_handler *
> -omgr_find_handler(const char *q, int qlen)
> +static int
> +omgr_oic_read_hdr(struct CborValue *cv, struct nmgr_hdr *out_hdr)
> {
> - char id_str[8];
> - int grp = -1;
> - int id = -1;
> - char *str;
> - char *eptr;
> - int slen;
> -
> - slen = oc_ri_get_query_value(q, qlen, "gr", &str);
> - if (slen > 0 && slen < sizeof(id_str) - 1) {
> - memcpy(id_str, str, slen);
> - id_str[slen] = '\0';
> - grp = strtoul(id_str, &eptr, 0);
> - if (*eptr != '\0') {
> - return NULL;
> - }
> + size_t hlen;
> + int rc;
> +
> + struct cbor_attr_t attrs[] = {
> + [0] = {
> + .attribute = "_h",
> + .type = CborAttrByteStringType,
> + .addr.bytestring.data = (void *)out_hdr,
> + .addr.bytestring.len = &hlen,
> + .nodefault = 1,
> + .len = sizeof *out_hdr,
> + },
> + [1] = { 0 }
> + };
> +
> + rc = cbor_read_object(cv, attrs);
> + if (rc != 0 || hlen != sizeof *out_hdr) {
> + return MGMT_ERR_EINVAL;
> }
> - slen = oc_ri_get_query_value(q, qlen, "id", &str);
> - if (slen > 0 && slen < sizeof(id_str) - 1) {
> - memcpy(id_str, str, slen);
> - id_str[slen] = '\0';
> - id = strtoul(id_str, &eptr, 0);
> - if (*eptr != '\0') {
> - return NULL;
> - }
> +
> + out_hdr->nh_len = ntohs(out_hdr->nh_len);
> + out_hdr->nh_group = ntohs(out_hdr->nh_group);
> +
> + return 0;
> +}
> +
> +static int
> +omgr_encode_nmp_hdr(struct CborEncoder *enc, struct nmgr_hdr hdr)
> +{
> + int rc;
> +
> + rc = cbor_encode_text_string(enc, "_h", 2);
> + if (rc != 0) {
> + return MGMT_ERR_ENOMEM;
> + }
> +
> + hdr.nh_len = htons(hdr.nh_len);
> + hdr.nh_group = htons(hdr.nh_group);
> +
> + /* Encode the NMP header in the response. */
> + rc = cbor_encode_byte_string(enc, (void *)&hdr, sizeof hdr);
> + if (rc != 0) {
> + return MGMT_ERR_ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +omgr_send_err_rsp(struct CborEncoder *enc, const struct nmgr_hdr *hdr,
> + int nmp_status)
> +{
> + int rc;
> +
> + rc = omgr_encode_nmp_hdr(enc, *hdr);
> + if (rc != 0) {
> + return rc;
> + }
> +
> + rc = cbor_encode_text_stringz(enc, "rc");
> + if (rc != 0) {
> + return MGMT_ERR_ENOMEM;
> + }
> +
> + rc = cbor_encode_int(enc, nmp_status);
> + if (rc != 0) {
> + return MGMT_ERR_ENOMEM;
> }
> - return mgmt_find_handler(grp, id);
> +
> + return 0;
> }
>
> static void
> -omgr_oic_op(oc_request_t *req, oc_interface_mask_t mask, int isset)
> +omgr_oic_put(oc_request_t *req, oc_interface_mask_t mask)
> {
> struct omgr_state *o = &omgr_state;
> const struct mgmt_handler *handler;
> uint16_t data_off;
> struct os_mbuf *m;
> int rc = 0;
> - extern CborEncoder g_encoder;
> + struct nmgr_hdr hdr;
> + int rsp_hdr_filled = 0;
>
> - if (!req->query_len) {
> - goto bad_req;
> - }
> -
> - handler = omgr_find_handler(req->query, req->query_len);
> - if (!handler) {
> - goto bad_req;
> - }
> + coap_get_payload(req->packet, &m, &data_off);
>
> - rc = coap_get_payload(req->packet, &m, &data_off);
> cbor_mbuf_reader_init(&o->os_cbuf.ob_reader, m, data_off);
> -
> cbor_parser_init(&o->os_cbuf.ob_reader.r, 0, &o->os_cbuf.ob_mj.parser,
> &o->os_cbuf.ob_mj.it);
>
> - /* start generating the CBOR OUTPUT */
> - /* this is worth a quick note. We are encoding CBOR within CBOR, so we
> - * need to use the same encoder as ocf stack. We are using their global
> - * encoder g_encoder which they intialized before this function is called.
> - * But we can't call their macros here as it won't use the right mape
> - * encoder (ob_mj) */
> + rc = omgr_oic_read_hdr(&o->os_cbuf.ob_mj.it, &hdr);
> + if (rc != 0) {
> + rc = MGMT_ERR_EINVAL;
> + goto done;
> + }
> +
> + /* Convert request header to response header to be sent. */
> + switch (hdr.nh_op) {
> + case NMGR_OP_READ:
> + hdr.nh_op = NMGR_OP_READ_RSP;
> + break;
> +
> + case NMGR_OP_WRITE:
> + hdr.nh_op = NMGR_OP_WRITE_RSP;
> + break;
> +
> + default:
> + goto done;
> + }
> + rsp_hdr_filled = 1;
> +
> + /* Begin root map in response. */
> cbor_encoder_create_map(&g_encoder, &o->os_cbuf.ob_mj.encoder,
> CborIndefiniteLength);
>
> + handler = mgmt_find_handler(hdr.nh_group, hdr.nh_id);
> + if (handler == NULL) {
> + rc = MGMT_ERR_ENOENT;
> + goto done;
> + }
> +
> + cbor_mbuf_reader_init(&o->os_cbuf.ob_reader, m, data_off);
> + cbor_parser_init(&o->os_cbuf.ob_reader.r, 0, &o->os_cbuf.ob_mj.parser,
> + &o->os_cbuf.ob_mj.it);
> +
> switch (mask) {
> case OC_IF_BASELINE:
> oc_process_baseline_interface(req->resource);
> + /* Fallthrough */
> +
> case OC_IF_RW:
> - if (!isset) {
> - cbor_encode_text_string(&root_map, "r", 1);
> - if (handler->mh_read) {
> - rc = handler->mh_read(&o->os_cbuf.ob_mj);
> + switch (hdr.nh_op) {
> + case NMGR_OP_READ_RSP:
> + if (handler->mh_read == NULL) {
> + rc = MGMT_ERR_ENOENT;
> } else {
> - goto bad_req;
> + rc = handler->mh_read(&o->os_cbuf.ob_mj);
> }
> - } else {
> - cbor_encode_text_string(&root_map, "w", 1);
> - if (handler->mh_write) {
> - rc = handler->mh_write(&o->os_cbuf.ob_mj);
> + break;
> +
> + case NMGR_OP_WRITE_RSP:
> + if (handler->mh_write == NULL) {
> + rc = MGMT_ERR_ENOENT;
> } else {
> - goto bad_req;
> + rc = handler->mh_write(&o->os_cbuf.ob_mj);
> }
> + break;
> +
> + default:
> + rc = MGMT_ERR_EINVAL;
> + break;
> }
> - if (rc) {
> - goto bad_req;
> + if (rc != 0) {
> + goto done;
> + }
> +
> + /* Encode the NMP header in the response. */
> + rc = omgr_encode_nmp_hdr(&o->os_cbuf.ob_mj.encoder, hdr);
> + if (rc != 0) {
> + rc = MGMT_ERR_ENOMEM;
> + goto done;
> }
> break;
> +
> default:
> break;
> }
>
> - cbor_encoder_close_container(&g_encoder, &o->os_cbuf.ob_mj.encoder);
> - oc_send_response(req, OC_STATUS_OK);
> + rc = 0;
>
> - return;
> -bad_req:
> - /*
> - * XXXX might send partially constructed response as payload
> - */
> - if (rc == MGMT_ERR_ENOMEM) {
> - rc = OC_STATUS_INTERNAL_SERVER_ERROR;
> - } else {
> - rc = OC_STATUS_BAD_REQUEST;
> - }
> - oc_send_response(req, rc);
> -}
> +done:
> + if (rc != 0) {
> + if (rsp_hdr_filled) {
> + rc = omgr_send_err_rsp(&g_encoder, &hdr, rc);
> + }
> + switch (rc) {
> + case 0:
> + break;
>
> -static void
> -omgr_oic_get(oc_request_t *req, oc_interface_mask_t mask)
> -{
> - omgr_oic_op(req, mask, 0);
> -}
> + case MGMT_ERR_ENOMEM:
> + rc = OC_STATUS_INTERNAL_SERVER_ERROR;
> + break;
>
> -static void
> -omgr_oic_put(oc_request_t *req, oc_interface_mask_t mask)
> -{
> - omgr_oic_op(req, mask, 1);
> + default:
> + rc = OC_STATUS_BAD_REQUEST;
> + break;
> + }
> + }
> +
> + cbor_encoder_close_container(&g_encoder, &o->os_cbuf.ob_mj.encoder);
> + oc_send_response(req, rc);
> }
>
> static void
> @@ -204,7 +277,6 @@ omgr_event_start(struct os_event *ev)
> oc_resource_bind_resource_interface(res, mode);
> oc_resource_set_default_interface(res, mode);
> oc_resource_set_discoverable(res);
> - oc_resource_set_request_handler(res, OC_GET, omgr_oic_get);
> oc_resource_set_request_handler(res, OC_PUT, omgr_oic_put);
> oc_add_resource(res);
> }
>