mkiiskila commented on a change in pull request #1917: Use SMP/OMP/MCUmgr,
remove newtmgr and change OICMGR to use OMP
URL: https://github.com/apache/mynewt-core/pull/1917#discussion_r330065932
##########
File path: mgmt/imgmgr/src/imgmgr.c
##########
@@ -558,530 +247,43 @@ imgr_erase_state(struct mgmt_cbuf *cb)
if (area_id >= 0) {
rc = flash_area_open(area_id, &fa);
if (rc) {
- return imgr_error_rsp(cb, MGMT_ERR_EINVAL,
- imgmgr_err_str_flash_open_failed);
+ return img_mgmt_error_rsp(ctxt, MGMT_ERR_EINVAL,
+ img_mgmt_err_str_flash_open_failed);
}
rc = flash_area_erase(fa, 0, sizeof(struct image_header));
if (rc) {
- return imgr_error_rsp(cb, MGMT_ERR_EINVAL,
- imgmgr_err_str_flash_erase_failed);
+ return img_mgmt_error_rsp(ctxt, MGMT_ERR_EINVAL,
+ img_mgmt_err_str_flash_erase_failed);
}
flash_area_close(fa);
-#if MYNEWT_VAL(LOG_FCB_SLOT1)
- /* If logging to slot1 is enabled, we can unlock it now. */
- if (area_id == FLASH_AREA_IMAGE_1) {
- log_fcb_slot1_unlock();
- }
-#endif
} else {
- return imgr_error_rsp(cb, MGMT_ERR_ENOMEM, imgmgr_err_str_no_slot);
+ return img_mgmt_error_rsp(ctxt, MGMT_ERR_ENOMEM,
img_mgmt_err_str_no_slot);
}
- g_err |= cbor_encode_text_stringz(&cb->encoder, "rc");
- g_err |= cbor_encode_int(&cb->encoder, MGMT_ERR_EOK);
+ g_err |= cbor_encode_text_stringz(&ctxt->encoder, "rc");
+ g_err |= cbor_encode_int(&ctxt->encoder, MGMT_ERR_EOK);
if (g_err) {
return MGMT_ERR_ENOMEM;
}
- /* Reset in-progress upload. */
- imgr_state.area_id = -1;
-
return 0;
}
-#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
-
-/**
- * Erases a flash sector as image upload crosses a sector boundary.
- * Erasing the entire flash size at one time can take significant time,
- * causing a bluetooth disconnect or significant battery sag.
- * Instead we will erase immediately prior to crossing a sector.
- * We could check for empty to increase efficiency, but instead we always erase
- * for consistency and simplicity.
- *
- * @param fa Flash area being traversed
- * @param off Offset that is about to be written
- * @param len Number of bytes to be written
- *
- * @return 0 if success
- * ERROR_CODE if could not erase sector
- */
-int
-imgr_erase_if_needed(const struct flash_area *fa, uint32_t off, uint32_t len)
-{
- int rc = 0;
- struct flash_area sector;
-
- while ((fa->fa_off + off + len) > imgr_state.sector_end) {
- rc = flash_area_getnext_sector(fa->fa_id, &imgr_state.sector_id,
§or);
- if (rc) {
- return rc;
- }
- rc = flash_area_erase(§or, 0, sector.fa_size);
- if (rc) {
- return rc;
- }
- imgr_state.sector_end = sector.fa_off + sector.fa_size;
- }
- return 0;
-}
-#endif
-
-/**
- * Verifies an upload request and indicates the actions that should be taken
- * during processing of the request. This is a "read only" function in the
- * sense that it doesn't write anything to flash and doesn't modify any global
- * variables.
- *
- * @param req The upload request to inspect.
- * @param action On success, gets populated with information
- * about how to process the request.
- *
- * @return 0 if processing should occur;
- * A MGMT_ERR code if an error response should be
- * sent instead.
- */
-static int
-imgr_upload_inspect(const struct imgr_upload_req *req,
- struct imgr_upload_action *action, const char **errstr)
-{
- const struct image_header *hdr;
- const struct flash_area *fa;
- struct image_version cur_ver;
- uint8_t rem_bytes;
- bool empty;
- int rc;
-
- memset(action, 0, sizeof *action);
-
- if (req->off == -1) {
- /* Request did not include an `off` field. */
- *errstr = imgmgr_err_str_hdr_malformed;
- return MGMT_ERR_EINVAL;
- }
-
- if (req->off == 0) {
- /* First upload chunk. */
- if (req->data_len < sizeof(struct image_header)) {
- /*
- * Image header is the first thing in the image.
- */
- *errstr = imgmgr_err_str_hdr_malformed;
- return MGMT_ERR_EINVAL;
- }
-
- if (req->size == -1) {
- /* Request did not include a `len` field. */
- *errstr = imgmgr_err_str_hdr_malformed;
- return MGMT_ERR_EINVAL;
- }
- action->size = req->size;
-
- hdr = (struct image_header *)req->img_data;
- if (hdr->ih_magic != IMAGE_MAGIC) {
- *errstr = imgmgr_err_str_magic_mismatch;
- return MGMT_ERR_EINVAL;
- }
-
- if (req->data_sha_len > IMGMGR_DATA_SHA_LEN) {
- return MGMT_ERR_EINVAL;
- }
-
- /*
- * If request includes proper data hash we can check whether there is
- * upload in progress (interrupted due to e.g. link disconnection) with
- * the same data hash so we can just resume it by simply including
- * current upload offset in response.
- */
- if ((req->data_sha_len > 0) && (imgr_state.area_id != -1)) {
- if ((imgr_state.data_sha_len == req->data_sha_len) &&
- !memcmp(imgr_state.data_sha, req->data_sha,
- req->data_sha_len)) {
- return 0;
- }
- }
-
- action->area_id = imgmgr_find_best_area_id();
- if (action->area_id < 0) {
- /* No slot where to upload! */
- *errstr = imgmgr_err_str_no_slot;
- return MGMT_ERR_ENOMEM;
- }
-
- if (req->upgrade) {
- /* User specified upgrade-only. Make sure new image version is
- * greater than that of the currently running image.
- */
- rc = imgr_my_version(&cur_ver);
- if (rc != 0) {
- return MGMT_ERR_EUNKNOWN;
- }
-
- if (imgr_vercmp(&cur_ver, &hdr->ih_ver) >= 0) {
- *errstr = imgmgr_err_str_downgrade;
- return MGMT_ERR_EBADSTATE;
- }
- }
-
-#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
- (void) empty;
-#else
- rc = flash_area_open(action->area_id, &fa);
- if (rc) {
- *errstr = imgmgr_err_str_flash_open_failed;
- return MGMT_ERR_EUNKNOWN;
- }
-
- rc = flash_area_is_empty(fa, &empty);
- flash_area_close(fa);
- if (rc) {
- return MGMT_ERR_EUNKNOWN;
- }
-
- action->erase = !empty;
-#endif
- } else {
- /* Continuation of upload. */
- action->area_id = imgr_state.area_id;
- action->size = imgr_state.size;
-
- if (req->off != imgr_state.off) {
- /*
- * Invalid offset. Drop the data, and respond with the offset we're
- * expecting data for.
- */
- return 0;
- }
- }
-
- /* Calculate size of flash write. */
- action->write_bytes = req->data_len;
- if (req->off + req->data_len < action->size) {
- /*
- * Respect flash write alignment if not in the last block
- */
- rc = flash_area_open(action->area_id, &fa);
- if (rc) {
- *errstr = imgmgr_err_str_flash_open_failed;
- return MGMT_ERR_EUNKNOWN;
- }
-
- rem_bytes = req->data_len % flash_area_align(fa);
- flash_area_close(fa);
-
- if (rem_bytes) {
- action->write_bytes -= rem_bytes;
- }
- }
-
- action->proceed = true;
- return 0;
-}
-
-static int
-imgr_upload_good_rsp(struct mgmt_cbuf *cb)
-{
- CborError err = CborNoError;
-
- err |= cbor_encode_text_stringz(&cb->encoder, "rc");
- err |= cbor_encode_int(&cb->encoder, MGMT_ERR_EOK);
- err |= cbor_encode_text_stringz(&cb->encoder, "off");
- err |= cbor_encode_int(&cb->encoder, imgr_state.off);
-
- if (err != 0) {
- return MGMT_ERR_ENOMEM;
- }
-
- return 0;
-}
-
-/**
- * Logs an upload request if necessary.
- *
- * @param is_first Whether the request includes the first chunk of
- * the image.
- * @param is_last Whether the request includes the last chunk of
- * the image.
- * @param status The result of processing the upload request
- * (MGMT_ERR code).
- *
- * @return 0 on success; nonzero on failure.
- */
-static int
-imgr_upload_log(bool is_first, bool is_last, int status)
-{
- uint8_t hash[IMGMGR_HASH_LEN];
- const uint8_t *hashp;
- int rc;
-
- if (is_first) {
- return imgmgr_log_upload_start(status);
- }
-
- if (is_last || status != 0) {
- /* Log the image hash if we know it. */
- rc = imgr_read_info(1, NULL, hash, NULL);
- if (rc != 0) {
- hashp = NULL;
- } else {
- hashp = hash;
- }
-
- return imgmgr_log_upload_done(status, hashp);
- }
-
- /* Nothing to log. */
- return 0;
-}
-
-static int
-imgr_upload(struct mgmt_cbuf *cb)
-{
- struct imgr_upload_req req = {
- .off = -1,
- .size = -1,
- .data_len = 0,
- .data_sha_len = 0,
- .upgrade = false,
- };
- const struct cbor_attr_t off_attr[] = {
- [0] = {
- .attribute = "data",
- .type = CborAttrByteStringType,
- .addr.bytestring.data = req.img_data,
- .addr.bytestring.len = &req.data_len,
- .len = sizeof(req.img_data)
- },
- [1] = {
- .attribute = "len",
- .type = CborAttrUnsignedIntegerType,
- .addr.uinteger = &req.size,
- .nodefault = true
- },
- [2] = {
- .attribute = "off",
- .type = CborAttrUnsignedIntegerType,
- .addr.uinteger = &req.off,
- .nodefault = true
- },
- [3] = {
- .attribute = "sha",
- .type = CborAttrByteStringType,
- .addr.bytestring.data = req.data_sha,
- .addr.bytestring.len = &req.data_sha_len,
- .len = sizeof(req.data_sha)
- },
- [4] = {
- .attribute = "upgrade",
- .type = CborAttrBooleanType,
- .addr.boolean = &req.upgrade,
- .dflt.boolean = false,
- },
- [5] = { 0 },
- };
- int rc;
- const char *errstr = NULL;
- struct imgr_upload_action action;
- const struct flash_area *fa = NULL;
-
- rc = cbor_read_object(&cb->it, off_attr);
- if (rc != 0) {
- return MGMT_ERR_EINVAL;
- }
-
- /* Determine what actions to take as a result of this request. */
- rc = imgr_upload_inspect(&req, &action, &errstr);
- if (rc != 0) {
- imgmgr_dfu_stopped();
- return rc;
- }
-
- if (!action.proceed) {
- /* Request specifies incorrect offset. Respond with a success code and
- * the correct offset.
- */
- return imgr_upload_good_rsp(cb);
- }
-
- /* Request is valid. Give the application a chance to reject this upload
- * request.
- */
- if (imgr_upload_cb != NULL) {
- rc = imgr_upload_cb(req.off, action.size, imgr_upload_arg);
- if (rc != 0) {
- errstr = imgmgr_err_str_app_reject;
- goto end;
- }
- }
-
- /* Remember flash area ID and image size for subsequent upload requests. */
- imgr_state.area_id = action.area_id;
- imgr_state.size = action.size;
-
- rc = flash_area_open(imgr_state.area_id, &fa);
- if (rc != 0) {
- rc = MGMT_ERR_EUNKNOWN;
- errstr = imgmgr_err_str_flash_open_failed;
- goto end;
- }
-
- if (req.off == 0) {
- /*
- * New upload.
- */
- imgr_state.off = 0;
-
- imgmgr_dfu_started();
-
- /*
- * We accept SHA trimmed to any length by client since it's up to
client
- * to make sure provided data are good enough to avoid collisions when
- * resuming upload.
- */
- imgr_state.data_sha_len = req.data_sha_len;
- memcpy(imgr_state.data_sha, req.data_sha, req.data_sha_len);
- memset(&imgr_state.data_sha[req.data_sha_len], 0,
- IMGMGR_DATA_SHA_LEN - req.data_sha_len);
-
-#if MYNEWT_VAL(LOG_FCB_SLOT1)
- /*
- * If logging to slot1 is enabled, make sure it's locked before
- * erasing so log handler does not corrupt our data.
- */
- if (imgr_state.area_id == FLASH_AREA_IMAGE_1) {
- log_fcb_slot1_lock();
- }
-#endif
-
-#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
- /* setup for lazy sector by sector erase */
- imgr_state.sector_id = -1;
- imgr_state.sector_end = 0;
-#else
- /* erase the entire req.size all at once */
- if (action.erase) {
- rc = flash_area_erase(fa, 0, req.size);
- if (rc != 0) {
- rc = MGMT_ERR_EUNKNOWN;
- errstr = imgmgr_err_str_flash_erase_failed;
- goto end;
- }
- }
-#endif
- }
-
- /* Write the image data to flash. */
- if (req.data_len != 0) {
-#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
- /* erase as we cross sector boundaries */
- if (imgr_erase_if_needed(fa, req.off, action.write_bytes) != 0) {
- rc = MGMT_ERR_EUNKNOWN;
- errstr = imgmgr_err_str_flash_erase_failed;
- goto end;
- }
-#endif
- rc = flash_area_write(fa, req.off, req.img_data, action.write_bytes);
- if (rc != 0) {
- rc = MGMT_ERR_EUNKNOWN;
- errstr = imgmgr_err_str_flash_write_failed;
- goto end;
- } else {
- imgr_state.off += action.write_bytes;
- if (imgr_state.off == imgr_state.size) {
- /* Done */
- imgmgr_dfu_pending();
- imgr_state.area_id = -1;
- }
- }
- }
-
-end:
- if (fa != NULL) {
- flash_area_close(fa);
- }
-
- imgr_upload_log(req.off == 0, imgr_state.off == imgr_state.size, rc);
-
- if (rc != 0) {
- imgmgr_dfu_stopped();
- return imgr_error_rsp(cb, rc, errstr);
- }
-
- return imgr_upload_good_rsp(cb);
-}
-
-void
-imgmgr_dfu_stopped(void)
-{
- if (imgmgr_dfu_callbacks_fn && imgmgr_dfu_callbacks_fn->dfu_stopped_cb) {
- imgmgr_dfu_callbacks_fn->dfu_stopped_cb();
- }
-}
-
-void
-imgmgr_dfu_started(void)
-{
- if (imgmgr_dfu_callbacks_fn && imgmgr_dfu_callbacks_fn->dfu_started_cb) {
- imgmgr_dfu_callbacks_fn->dfu_started_cb();
- }
-}
-
-void
-imgmgr_dfu_pending(void)
-{
- if (imgmgr_dfu_callbacks_fn && imgmgr_dfu_callbacks_fn->dfu_pending_cb) {
- imgmgr_dfu_callbacks_fn->dfu_pending_cb();
- }
-}
-
-void
-imgmgr_dfu_confirmed(void)
-{
- if (imgmgr_dfu_callbacks_fn && imgmgr_dfu_callbacks_fn->dfu_confirmed_cb) {
- imgmgr_dfu_callbacks_fn->dfu_confirmed_cb();
- }
-}
-
-void
-imgr_set_upload_cb(imgr_upload_fn *cb, void *arg)
-{
- imgr_upload_cb = cb;
- imgr_upload_arg = arg;
-}
-
-void
-imgmgr_register_callbacks(const imgmgr_dfu_callbacks_t *cb_struct)
-{
- imgmgr_dfu_callbacks_fn = cb_struct;
-}
-
void
imgmgr_module_init(void)
{
- int rc;
-
/* Ensure this function only gets called by sysinit. */
SYSINIT_ASSERT_ACTIVE();
- rc = mgmt_group_register(&imgr_nmgr_group);
- SYSINIT_PANIC_ASSERT(rc == 0);
+ mgmt_register_group(&imgr_mgmt_group);
#if MYNEWT_VAL(IMGMGR_CLI)
+ int rc;
Review comment:
Not a fan of variable declarations deep inside the function.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
