Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LWM2M: fix handle_reqest() errors #1273

Merged
merged 4 commits into from
Oct 19, 2017
Merged

LWM2M: fix handle_reqest() errors #1273

merged 4 commits into from
Oct 19, 2017

Conversation

rbtchc
Copy link
Collaborator

@rbtchc rbtchc commented Aug 28, 2017

  1. Refactor the error handling section (split from Add LwM2M firmware push/pull function #1049)
  2. Respond 4.5 (Method Not Allowed) when option PATH is not given.
    '/' is only allowed when performing bootstrap-delete
  3. Respond 4.4 (Not found) when object doesn't exist
  4. Respond 4.4 (Not found) when URI contains character other than digits
  5. Respond 5.0 (Internal server error) when OP is not supported
  6. Respond 4.5 (Method Not Allowed) when request method is not GET for URI .well-know/core

@rbtchc rbtchc force-pushed the lwm2m_handle_request branch from d76f801 to bbbc803 Compare August 28, 2017 12:25
tbursztyka
tbursztyka requested changes Aug 28, 2017
View reviewed changes
Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some description of what the patch does in commit messages, for instance the last one: sure it does what it's commit title says, but why? A little bit of background is nice for git history.

subsys/net/lib/lwm2m/lwm2m_engine.c
@@ -2029,7 +2029,8 @@ static int handle_request(struct zoap_packet *request,
obj = get_engine_obj(path.obj_id);
if (!obj) {
/* No matching object found - ignore request */
return -ENOENT;
r = -ENOENT;
goto error;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch could be merged to the first one:
"refactor handle_request() error handling "

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

subsys/net/lib/lwm2m/lwm2m_engine.c
@@ -2158,7 +2158,7 @@ static int handle_request(struct zoap_packet *request,

default:
SYS_LOG_ERR("Unknown operation: %u", context.operation);
return -EINVAL;
r = -EINVAL;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that one could be merged with the first of this patch-set too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rbtchc rbtchc force-pushed the lwm2m_handle_request branch from bbbc803 to a0b54af Compare August 28, 2017 14:54
@rbtchc
Copy link
Collaborator Author

rbtchc commented Aug 28, 2017

Update according to @tbursztyka 's review.
Fold 3, 5 into 1 and add more description to the commits.

@rbtchc
Copy link
Collaborator Author

rbtchc commented Aug 29, 2017

@jukkar , @mike-scott please help to take a look at the patches. thanks

@mike-scott
Copy link
Contributor

Will do later. Sorry for delay. I had too many patches in too many branches today.

@mike-scott
Copy link
Contributor

Changes look good. We'll need to work out a merge order for this PR, the net_app update (#1255) and firmware update (#1049)

@mike-scott
Copy link
Contributor

@rbtchc Won't this patch conflict with changes in your other PR: #1049 ?

mike-scott
mike-scott reviewed Aug 29, 2017
View reviewed changes
Copy link
Contributor

@mike-scott mike-scott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing approval for now till we work out a merge order.

mike-scott
mike-scott suggested changes Aug 29, 2017
View reviewed changes
Copy link
Contributor

@mike-scott mike-scott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er.. approval removing..

@rbtchc
Copy link
Collaborator Author

rbtchc commented Aug 30, 2017

@mike-scott I think so. I suggest that we can merge #1255 first and then this one and then #1049.

@jukkar jukkar self-requested a review September 1, 2017 14:46
@jukkar jukkar added the net label Sep 1, 2017
rbtchc added 4 commits October 18, 2017 09:52
@rbtchc
net: lwm2m: response nicely when obj not found or OP not handled
a8346fe 
1) Respond NOT FOUND to caller when object doesn't exist
2) Report as internal server error when OP not handled

Signed-off-by: Robert Chou <robert.ch.chou@acer.com>
@rbtchc
net: lwm2m: fix empty path being treated as 0/0/0 issue
96ed033 
Return 4.05 Method Not Allowed when path is empty ('/') to the
caller for it's only use by bootstrap delete. This change also avoid the
empty path being treated as request targeted at 0/0/0.

Signed-off-by: Robert Chou <robert.ch.chou@acer.com>
@rbtchc
net: lwm2m: return 4.04 when URI is not composed by digits
c491977 
Modify zoap_options_to_path() to return error when URI contains
character other than digits and return 4.04 NOT FOUND to caller.

PATH such as "/1a/2/3" was treated as "/1/2/3" after parsring
which is incorrect.

Signed-off-by: Robert Chou <robert.ch.chou@acer.com>
@rbtchc
net: lwm2m: only method GET is allowed for .well-known/core
3b7b766 
".well-known/core" is mainly used with method GET for performing the
resource discovery (RFC 6690). Since we are implementing a LwM2M client
and is not implement a resource directory which allow others to do the
resource registration (POST to .well-known/core). Only GET method is
allowed for the usage. Report 4.5 (Method Not Allowed) if other methods
are requested.

Signed-off-by: Robert Chou <robert.ch.chou@acer.com>
@rbtchc rbtchc force-pushed the lwm2m_handle_request branch from a0b54af to 3b7b766 Compare October 18, 2017 02:10
@rbtchc
Copy link
Collaborator Author

rbtchc commented Oct 18, 2017

Rebase on top of the master branch for #4380 is already merged.

jukkar
jukkar approved these changes Oct 18, 2017
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mike-scott
mike-scott approved these changes Oct 18, 2017
View reviewed changes
Copy link
Contributor

@mike-scott mike-scott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rbtchc
Copy link
Collaborator Author

rbtchc commented Oct 18, 2017

@tbursztyka could you please help to take a look at commits and share your opinions? thanks

@jukkar
Copy link
Member

jukkar commented Oct 19, 2017

@tbursztyka could you please help to take a look at commits and share your opinions? thanks

Tomasz is on vacation still next week. I try to get @nashif to merge this one as @tbursztyka comments were addressed in later versions of the PR.

@jukkar jukkar requested a review from nashif October 19, 2017 07:31
@nashif nashif merged commit 652cc72 into zephyrproject-rtos:master Oct 19, 2017
@rbtchc rbtchc deleted the lwm2m_handle_request branch November 1, 2017 06:43
nagineni pushed a commit to nagineni/zephyr that referenced this pull request Nov 20, 2017
@grgustaf
[json] Support JSON using JerryScript (zephyrproject-rtos#1273)
6a01f09 
Fixes zephyrproject-rtos#1282

Signed-off-by: Jimmy Huang <jimmy.huang@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbursztyka tbursztyka tbursztyka requested changes

@mike-scott mike-scott mike-scott approved these changes

@jukkar jukkar jukkar approved these changes

@nashif nashif Awaiting requested review from nashif

Assignees
No one assigned
Labels
area: Networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rbtchc @mike-scott @jukkar @tbursztyka @nashif