Add U-Boot userspace tools support #9

pasinskim · 2016-01-08T17:20:44Z

No description provided.

Signed-off-by: Marcin Pasinski <marcin.pasinski@mender.io>

pasinskim · 2016-01-11T10:05:54Z

@maciejmrowiec can you take a look at this?

maciejmrowiec · 2016-01-11T10:27:21Z

bootenvutil.go

+
+type RealRunner struct{}
+
+var runner Runner


This is required to be configured on package load?

What do you mean? This must be set before calling any function from module.

var staring from lower letter is private to the package, if this will be imported from another library it will me impossible to set initial runner instance. Should be public or have a setter function.

can use init function to set the default runner

func init() {
runner = blah blah
}

this function will be executed when the library is imported for the first time, this way it will never happen that you call runner.Run without setting it to something.

What do you mean? This must be set before calling any function from module.

func main() {
env, _ := GetBootEnv("MyVar") // internally runner is inited to nil, and it calls nil.Command()
// RUNTIME PANNIC

// correct version:
runner = RealRunner{}
env, _ := GetBootEnv("MyVar")
}

maciejmrowiec · 2016-01-11T10:41:56Z

I think in general utility should not print anything to error stream but pass all required error information higher in the stack.

For logging try to use standard logging (can be configured globally with timestamps and stuff).

import "log"

in main: log.SetOutput(os.Stderr)
log.Println(err)or for exit error log.Fatal(err) anywhere

maciejmrowiec · 2016-01-11T10:53:17Z

bootenvutil.go

+	return env_variables, err
+}
+
+func GetBootEnv(var_name ...string) (map[string]string, error) {


make map[string]string a dedicated type, don't have to remember this complex construct, also later type will have to be modified it's only one place to do this

type EnvVariablesBlah map[string]string

func GetBootEnv(var_name ...string) (EnvVariablesBlah, error)

Modify SetBootEnv() to return err on failure. Use log instead of printing directly to stdout or stderr. Add initialization of runner in init() function.

kacf · 2016-01-12T08:45:42Z

Btw, shouldn't all this be inside src/github.com/mendersoftware? That is, if I understand the directory guidelines correctly.

maciejmrowiec · 2016-01-12T09:53:49Z

@kacf not sure if I follow what you mean?

kacf · 2016-01-12T11:53:39Z

@maciejmrowiec: I thought that source code should always be inside a directory like that, and not directly in the repository root. But I may have misunderstood it.

kacf · 2016-01-12T11:55:27Z

bootenvutil.go

+	return exec.Command(command, args...)
+}
+
+func (c *UbootEnvCommand) Command(params ...string) (UbootVars, error) {


Another thing: Should functions like these start with an upper case? As long as we're not exporting an API, I presume everything should be lowercase, so we don't export anything? If we ever move this into a library it might be nice to not have to clean all this up later.

maciejmrowiec · 2016-01-12T13:37:48Z

@kacf you checkout repository to this directory structure

maciejmrowiec · 2016-01-12T13:38:04Z

I'm ok with merging

Add U-Boot userspace tools support

Once in a while, in release mode only, this test will display this symptom: ``` ... record_id=163 severity=trace time="2023-Oct-03 16:22:53.911616" name="http_client" url="http://127.0.0.1:8001" msg="Read 16384 bytes of body data from stream." record_id=164 severity=trace time="2023-Oct-03 16:22:53.911802" name="http_client" url="http://127.0.0.1:8001" msg="Read 16384 bytes of body data from stream." record_id=165 severity=warning time="2023-Oct-03 16:22:53.912043" name="http_client" url="http://127.0.0.1:8001" msg="Client destroyed while request is still active!" [ OK ] HttpTest.TestResponseBody (202 ms) [----------] 1 test from HttpTest (202 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test suite ran. (202 ms total) [ PASSED ] 1 test. corrupted double-linked list Aborted (core dumped) ``` The backtrace reveals that it happens at the very very end, when exit handlers are called: ``` Program terminated with signal SIGABRT, Aborted. #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=139805181667136) at ./nptl/pthread_kill.c:44 44 ./nptl/pthread_kill.c: No such file or directory. (gdb) bt #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=139805181667136) at ./nptl/pthread_kill.c:44 #1 __pthread_kill_internal (signo=6, threadid=139805181667136) at ./nptl/pthread_kill.c:78 #2 __GI___pthread_kill (threadid=139805181667136, signo=signo@entry=6) at ./nptl/pthread_kill.c:89 #3 0x00007f26ee375476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #4 0x00007f26ee35b7f3 in __GI_abort () at ./stdlib/abort.c:79 #5 0x00007f26ee3bc6f6 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7f26ee50eb8c "%s\n") at ../sysdeps/posix/libc_fatal.c:155 #6 0x00007f26ee3d3d7c in malloc_printerr ( str=str@entry=0x7f26ee50c72e "corrupted double-linked list") at ./malloc/malloc.c:5664 #7 0x00007f26ee3d484c in unlink_chunk (p=<optimized out>, av=0x7f26ee54cc80 <main_arena>) at ./malloc/malloc.c:1635 #8 0x00007f26ee3d49e9 in malloc_consolidate ( av=av@entry=0x7f26ee54cc80 <main_arena>) at ./malloc/malloc.c:4780 #9 0x00007f26ee3d5f20 in _int_free (av=0x7f26ee54cc80 <main_arena>, p=0x561b9a7adae0, have_lock=<optimized out>) at ./malloc/malloc.c:4674 #10 0x00007f26ee3d84d3 in __GI___libc_free (mem=<optimized out>) at ./malloc/malloc.c:3391 #11 0x00007f26eeb2017d in ?? () from /lib/x86_64-linux-gnu/libcrypto.so.3 #12 0x00007f26eeb44d0d in ?? () from /lib/x86_64-linux-gnu/libcrypto.so.3 #13 0x00007f26eeb1b1d5 in CRYPTO_free_ex_data () from /lib/x86_64-linux-gnu/libcrypto.so.3 #14 0x00007f26eeb13d1f in ?? () from /lib/x86_64-linux-gnu/libcrypto.so.3 #15 0x00007f26eeb1d929 in OPENSSL_cleanup () from /lib/x86_64-linux-gnu/libcrypto.so.3 #16 0x00007f26ee378495 in __run_exit_handlers (status=0, listp=0x7f26ee54c838 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at ./stdlib/exit.c:113 #17 0x00007f26ee378610 in __GI_exit (status=<optimized out>) at ./stdlib/exit.c:143 #18 0x00007f26ee35cd97 in __libc_start_call_main ( main=main@entry=0x561b9a0c0f70 <main(int, char**)>, argc=argc@entry=2, argv=argv@entry=0x7ffe48d637c8) at ../sysdeps/nptl/libc_start_call_main.h:74 #19 0x00007f26ee35ce40 in __libc_start_main_impl ( main=0x561b9a0c0f70 <main(int, char**)>, argc=2, argv=0x7ffe48d637c8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffe48d637b8) at ../csu/libc-start.c:392 #20 0x0000561b9a0c1a35 in _start () ``` It is unknown what causes the corruption, and the problem only happens in release mode with sanitizers disabled, so it's very hard to investigate. But although the root cause isn't known, it's believed to happen when the body has not been completely consumed, and the program exits. Since this "don't-consume -> then exit" scenario is very unlikely in production, work around it by making sure both handlers have run before exiting, instead of only one of them. I tested this for hundreds of runs, and it worked. Previously it would fail every 15-30 runs or so. This also has the added benefit of not accidentally skipping the test conditionals inside the body handler. Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech>

pasinskim added 5 commits January 8, 2016 18:18

Add U-Boot env modification.

0110132

Add full fw_setenv and fw_getenv wrapper implementation.

f47a9c0

Move use cases description to test file.

1ec239f

Signed-off-by: Marcin Pasinski <marcin.pasinski@mender.io>

Add exec interface and command runner for mocking tests.

5cc25a2

Format code with go fmt.

ef2f019

pasinskim force-pushed the master branch from 2d26659 to ef2f019 Compare January 8, 2016 17:35

Fix SetBootEnv() function and refactor tests.

eb468b0

maciejmrowiec reviewed Jan 11, 2016
View reviewed changes

pasinskim added 2 commits January 11, 2016 13:21

Fix review issues.

4f601a8

Modify SetBootEnv() to return err on failure. Use log instead of printing directly to stdout or stderr. Add initialization of runner in init() function.

Simplify types used in bootenvutil module.

ec72778

kacf reviewed Jan 12, 2016
View reviewed changes

pasinskim force-pushed the master branch from 3d528b2 to ec72778 Compare January 12, 2016 13:30

Change name of local function to be not exported from module.

d2f192d

Merge branch 'master' into master

b42d05d

pasinskim added a commit that referenced this pull request Jan 12, 2016

Merge pull request #9 from pasinskim/master

7a311a4

Add U-Boot userspace tools support

pasinskim merged commit 7a311a4 into mendersoftware:master Jan 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add U-Boot userspace tools support #9

Add U-Boot userspace tools support #9

pasinskim commented Jan 8, 2016

pasinskim commented Jan 11, 2016

maciejmrowiec Jan 11, 2016

pasinskim Jan 11, 2016

maciejmrowiec Jan 11, 2016

maciejmrowiec Jan 11, 2016

maciejmrowiec Jan 11, 2016

maciejmrowiec commented Jan 11, 2016

maciejmrowiec Jan 11, 2016

kacf commented Jan 12, 2016

maciejmrowiec commented Jan 12, 2016

kacf commented Jan 12, 2016

kacf Jan 12, 2016

maciejmrowiec commented Jan 12, 2016

maciejmrowiec commented Jan 12, 2016

Add U-Boot userspace tools support #9

Add U-Boot userspace tools support #9

Conversation

pasinskim commented Jan 8, 2016

pasinskim commented Jan 11, 2016

maciejmrowiec Jan 11, 2016

Choose a reason for hiding this comment

pasinskim Jan 11, 2016

Choose a reason for hiding this comment

maciejmrowiec Jan 11, 2016

Choose a reason for hiding this comment

maciejmrowiec Jan 11, 2016

Choose a reason for hiding this comment

maciejmrowiec Jan 11, 2016

Choose a reason for hiding this comment

maciejmrowiec commented Jan 11, 2016

maciejmrowiec Jan 11, 2016

Choose a reason for hiding this comment

kacf commented Jan 12, 2016

maciejmrowiec commented Jan 12, 2016

kacf commented Jan 12, 2016

kacf Jan 12, 2016

Choose a reason for hiding this comment

maciejmrowiec commented Jan 12, 2016

maciejmrowiec commented Jan 12, 2016