LwM2M: fix block transfer with TLV #34936

jb-gcx · 2021-05-06T15:25:48Z

Follow-up packets do not contain the TLV header but just continuations of the value. Tested with a Leshan server and a custom opaque resource.

Fixes #34935

Signed-off-by: Jan Bünker jan.buenker@grandcentrix.net

rlubos

Thanks, current implementation indeed was limited to firmware update only, thanks for making this more generic. It also leaves an opening for further improvements in the future, as currently block transfer writes are limited to a single resource only.

You'd need to update your commit message in order to satisfy Zephyr CI - please add net: lwm2m: prefix to the commit title, a signoff entry and stick to char/line limit of the commit message. You can check what's wrong in the compliance check results.

rlubos · 2021-05-07T08:12:05Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

Please move the variable declaration to the beginning of the block.

rlubos · 2021-05-07T08:12:49Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

Would be nice to keep the indentation from the original code.

rlubos · 2021-05-07T08:15:05Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

We typically avoid assignments inside the if condition

Suggested change

if ((ret = write_tlv_resource(msg, &tlv2))) {

ret = write_tlv_resource(msg, &tlv2);

if (ret < 0) {

rlubos · 2021-05-07T08:15:17Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

rlubos

One more nit, there's a duplicated signoff entry in the commit message, but otherwise looks good.

jb-gcx · 2021-05-07T14:02:24Z

Thanks for the Feedback @rlubos ! After editing the existing commit message, git appended the signoff message a second time and I didn't notice. I hope everything is in order now.

jukkar

LGTM

rlubos · 2021-05-10T07:51:36Z

@jukkar Any idea why is the gitlint complaining about the signoff entry? It looks correct to me...

jb-gcx · 2021-05-10T08:10:31Z

The original message had the signoff entry as well, and it also complained. I figured gitlint might have gotten confused because of other issues with the message, but those are fixed now.

Maybe it doesn't like the umlaut?

rlubos · 2021-05-10T08:26:40Z

The original message had the signoff entry as well, and it also complained. I figured gitlint might have gotten confused because of other issues with the message, but those are fixed now.

Maybe it doesn't like the umlaut?

I'm not sure to be honest - it doesn't seem like any of the typical issues I've seen so far. gitlint is fairly easy to run locally, perhaps you could give it a show w/o umlaut. Just not that another gitlint requirement is that the commit author name matches the signoff entry, so you might need to run --reset-author option on the commit as well if you modify it.

jukkar · 2021-05-10T08:31:05Z

The exact message is 1: UC2 Signed-off-by: must have a full name but you have a full name there so this is quite weird.

Maybe it doesn't like the umlaut?

Hmm, I am seeing chars like ö used in signed-off-by lines in the log so it is probably not that.

@galak any idea what is going on here?

jb-gcx · 2021-05-10T10:38:47Z

I saw the issue locally, amended the commit without any changes and without -s, then re-ran gitlint and it worked. If this works, then the cause seems to be gits automatic signoff feature, although I don't understand why.

rlubos · 2021-05-10T11:23:30Z

@jb-gcx Now the complaint is different:

author email (Jan Bünker jan.buenker@grandcentrix.net) needs to match one of the signed-off-by entries.

jb-gcx · 2021-05-10T12:10:02Z

I'm stumped by this one. My local gitlint is OK with everything, and the git log output looks correct as well:

commit 55e6c527b312eafc28f467058906e93035a4ba76 (HEAD -> fix_lwm2m_block_transfer_tlv, jb-gcx-fork/fix_lwm2m_block_transfer_tlv)
Author: Jan Bünker <jan.buenker@grandcentrix.net>
Date:   Tue May 4 16:43:49 2021 +0200

    net: lwm2m: Only parse TLV from the first block
    
    This was already implemented for firmware update packages.
    For other opaque resources it failed to determine the target resource
    id, which is now stored in the block_context.
    
    Signed-off-by: Jan Bünker <jan.buenker@grandcentrix.net>

Curious: the output of the online check references b6b1733d7e895f227ce255d1ee73caf9b222c720 - if that's supposed to be a commit hash, it's the wrong one.

rlubos · 2021-05-10T12:46:40Z

Curious: the output of the online check references b6b1733d7e895f227ce255d1ee73caf9b222c720 - if that's supposed to be a commit hash, it's the wrong one.

It's likely due to the fact, that compliance check rebases the PR branch to the latest master before running the checks - hence different SHA of your commit.

I'm not gitlint expert, but from experience I can tell that it only reports results if the gitlint command was issued from the root directory of the repository - perhaps you did otherwise and that's why you can't see the error locally?

jb-gcx · 2021-05-10T12:50:38Z

I'm not gitlint expert, but from experience I can tell that it only reports results if the gitlint command was issued from the root directory of the repository - perhaps you did otherwise and that's why you can't see the error locally?

Unfortunately no, I'm in the root directory. I've tried rebasing onto master and pushing again, maybe this will help. I thought maybe the rebase modifies the author somehow. Although then this error would occur a lot, so it doesn't seem likely.

rlubos · 2021-05-10T12:56:02Z

Indeed, I've just tried locally on my side and I cannot reproduce the error as well... I'm totally out of ideas right now, @galak @carlescufi @nashif any clues what could be the gitlint problem here?

carlescufi · 2021-05-10T16:26:15Z

Indeed, I've just tried locally on my side and I cannot reproduce the error as well... I'm totally out of ideas right now, @galak @carlescufi @nashif any clues what could be the gitlint problem here?

I can reproduce the error:

carles@zephyr-nrf ~/src/zephyr/zephyr (pr/34936)
$  ./scripts/ci/check_compliance.py -m Codeowners -m Devicetree -m Gitlint -m Identity -m Nits -m pylint -m checkpatch -m Kconfig -c HEAD~..
Running checkpatch       tests in /home/carles/src/zephyr/zephyr ...
Running Kconfig          tests in None ...
Running Codeowners       tests in /home/carles/src/zephyr/zephyr ...
Running Nits             tests in /home/carles/src/zephyr/zephyr ...
Running Gitlint          tests in /home/carles/src/zephyr/zephyr ...
Running pylint           tests in /home/carles/src/zephyr/zephyr ...
Running Identity         tests in /home/carles/src/zephyr/zephyr ...
WARNING : Skipped Kconfig, Not a Zephyr tree (ZEPHYR_BASE unset)
1 checks failed
ERROR   : Test Identity failed: 1e07cb4490871ea95948ec71e4255f76ce97f8bc: author email (Jan Bünker <jan.buenker@grandcentrix.net>) needs to match one of the signed-off-by entries.

Complete results in compliance.xml

But I am unsure what is causing it, the entries seem correct in both places.

jb-gcx · 2021-05-10T16:29:10Z

Maybe there is a difference in behavior between linux and macos? I am on mac OS.

carlescufi · 2021-05-10T16:38:24Z

It's an encoding problem:

Your email: b'Jan Bu\xcc\x88nker <jan.buenker@grandcentrix.net>'
Your signed-off-by line: b'Jan B\xc3\xbcnker <jan.buenker@grandcentrix.net>'

The first one is using an actual ASCII u character and then this modifier and the second is using this one character

jb-gcx · 2021-05-10T17:21:58Z

It's an encoding problem:

Your email: b'Jan Bu\xcc\x88nker <jan.buenker@grandcentrix.net>'
Your signed-off-by line: b'Jan B\xc3\xbcnker <jan.buenker@grandcentrix.net>'

The first one is using an actual ASCII u character and then this modifier and the second is using this one character

Thanks, I didn't see that!

I am now using the same encoding as the author field in the commit message, but the old error is back: 1: UC2 Signed-off-by: must have a full name. I guess it doesn't like that encoding.

But now I know why it worked one time, when I had the double signoff message; the two lines must have been encoded differently, so gitlint used one encoding to compare to the author and the other one to check for a full name.

I'm out of time for today, but tomorrow I'll either change the encoding of my author field, or just use "Buenker" everywhere to make myself future proof.

This was already implemented for firmware update packages. For other opaque resources it failed to determine the target resource id, which is now stored in the block_context. Signed-off-by: Jan Buenker <jan.buenker@grandcentrix.net>

ziposoft-ac · 2022-05-12T23:24:51Z

subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

 			return ret;
 		}
+
+		return 0;


This "return 0;" seems to cause a problem.
If the current packet contains writes to other resources in the object, those will not be processed. Only the first item is written, and function returns without processing the rest.
If I remove the "return 0", it seems to resolve my problem and the other writes are processed. However, that "return 0" was specifically added for a reason, so I am not confident that is the correct fix.

jb-gcx requested a review from rlubos as a code owner May 6, 2021 15:25

github-actions bot added the area: Networking label May 6, 2021

rlubos reviewed May 7, 2021

View reviewed changes

rlubos requested a review from jukkar May 7, 2021 09:17

jukkar added this to the v2.6.0 milestone May 7, 2021

jb-gcx force-pushed the fix_lwm2m_block_transfer_tlv branch from ec8d57e to 498e762 Compare May 7, 2021 13:12

rlubos approved these changes May 7, 2021

View reviewed changes

galak added the bug The issue is a bug, or the PR is fixing a bug label May 7, 2021

jb-gcx force-pushed the fix_lwm2m_block_transfer_tlv branch from 498e762 to 9c7ea52 Compare May 7, 2021 13:59

jukkar approved these changes May 8, 2021

View reviewed changes

jb-gcx force-pushed the fix_lwm2m_block_transfer_tlv branch from 9c7ea52 to 55e6c52 Compare May 10, 2021 10:35

jb-gcx force-pushed the fix_lwm2m_block_transfer_tlv branch from 55e6c52 to 1e07cb4 Compare May 10, 2021 12:48

jb-gcx force-pushed the fix_lwm2m_block_transfer_tlv branch from 1e07cb4 to 52341fd Compare May 10, 2021 16:50

net: lwm2m: Only parse TLV from the first block

d2ee8f4

This was already implemented for firmware update packages. For other opaque resources it failed to determine the target resource id, which is now stored in the block_context. Signed-off-by: Jan Buenker <jan.buenker@grandcentrix.net>

jb-gcx force-pushed the fix_lwm2m_block_transfer_tlv branch from 52341fd to d2ee8f4 Compare May 11, 2021 12:39

zephyrbot added the area: LWM2M label May 11, 2021

zephyrbot assigned rlubos May 11, 2021

nashif merged commit 6815ef4 into zephyrproject-rtos:master May 14, 2021

ziposoft-ac reviewed May 12, 2022

View reviewed changes

	if ((ret = write_tlv_resource(msg, &tlv2))) {
	ret = write_tlv_resource(msg, &tlv2);
	if (ret < 0) {

LwM2M: fix block transfer with TLV #34936

LwM2M: fix block transfer with TLV #34936

Uh oh!

Conversation

jb-gcx commented May 6, 2021

Uh oh!

rlubos left a comment

Choose a reason for hiding this comment

Uh oh!

rlubos May 7, 2021

Choose a reason for hiding this comment

Uh oh!

rlubos May 7, 2021

Choose a reason for hiding this comment

Uh oh!

rlubos May 7, 2021

Choose a reason for hiding this comment

Uh oh!

rlubos May 7, 2021

Choose a reason for hiding this comment

Uh oh!

rlubos left a comment

Choose a reason for hiding this comment

Uh oh!

jb-gcx commented May 7, 2021

Uh oh!

jukkar left a comment

Choose a reason for hiding this comment

Uh oh!

rlubos commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jb-gcx commented May 10, 2021

Uh oh!

rlubos commented May 10, 2021

Uh oh!

jukkar commented May 10, 2021

Uh oh!

jb-gcx commented May 10, 2021

Uh oh!

rlubos commented May 10, 2021

Uh oh!

jb-gcx commented May 10, 2021

Uh oh!

rlubos commented May 10, 2021

Uh oh!

jb-gcx commented May 10, 2021

Uh oh!

rlubos commented May 10, 2021

Uh oh!

carlescufi commented May 10, 2021

Uh oh!

jb-gcx commented May 10, 2021

Uh oh!

carlescufi commented May 10, 2021

Uh oh!

jb-gcx commented May 10, 2021

Uh oh!

ziposoft-ac May 12, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rlubos commented May 10, 2021 •

edited

Loading