tools: share internal API symbols #2437

2xsec · 2018-06-28T14:26:47Z

Hello,

I'm working to share internal API symbols to tools.

I'll add commit for other tools to this pull request.

Before that, if you have any opinion for that, Please let us know.

Thanks.

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

lxc-jenkins · 2018-06-28T14:26:48Z

This pull request didn't trigger Jenkins as its author isn't in the whitelist.

An organization member must perform one of the following:

To have this branch tested by Jenkins, use the "ok to test" command.
To have a one time test done, use the "test this please" command.

Those commands are simple Github comments of the format: "jenkins: COMMAND"

brauner · 2018-06-28T14:30:33Z

jenkins: test this please

brauner · 2018-06-28T20:51:47Z

jenkins: test this please

brauner · 2018-06-29T08:24:27Z

@2xsec, just ping me when you think this branch is ready to be merged. :)

2xsec · 2018-06-29T11:39:21Z

OK^^

brauner · 2018-06-29T13:05:50Z

Note, that we should keep symbols that are only used by the tools restricted to the tools, i.e. they should not appear in the dependency list or files for the shared library itself. :)

brauner · 2018-06-29T13:08:32Z

Also, in case you're at OSS NA or OSS EU, or Linux Plumbers you've earned a drink. :)

2xsec · 2018-06-29T14:52:42Z

Dear Mr.Brauner,

All works are finished. Some tools have still issues related logs. But I think it is not critical issue.

I'll fix them later. And some apis are still duplicated like lxc_tool_monitord_spawn, lxc_tool_check_inherited in lxc_monitor.c.

Please let us know your opinion.

Thanks^^

brauner · 2018-06-29T15:11:04Z

Please, no Mr. necessary I'm far away from that. :)
Cool, I'll give this a look tomorrow and if all checks out I'll merge.

brauner · 2018-06-29T16:11:04Z

Hm, lxc-test-autostart seems unhappy. :)

2xsec · 2018-06-30T02:37:46Z

I'll check it.

brauner · 2018-06-30T09:23:22Z

@2xsec, can you merge the last commit into the other lxc-autostart commit, please?

2xsec · 2018-06-30T09:26:47Z

@brauner, I didn't understand. How do I for that?

brauner · 2018-06-30T09:38:32Z

@2xsec,
:)

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

2xsec · 2018-06-30T09:49:22Z

@brauner It's done. But, push is failed.

guybrush@nt900x5t:~/Project/github/lxc$ git push origin bugfix
Username for 'https://github.com': 2xsec
Password for 'https://2xsec@github.com':
To https://github.com/2xsec/lxc.git
! [rejected] bugfix -> bugfix (non-fast-forward)
error: failed to push some refs to 'https://github.com/2xsec/lxc.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

brauner · 2018-06-30T09:55:39Z

@2xsec use git push --force in such cases. :)

2xsec · 2018-06-30T10:00:58Z

@brauner Thanks^^

2xsec · 2018-06-30T10:26:29Z

apparmor-mount TC is failed. I'll check it again.

FAIL: lxc-tests: /usr/bin/lxc-test-apparmor-mount

brauner · 2018-06-30T10:30:28Z

No, you can ignore that error. It's caused by:

Setting up the GPG keyring
ERROR: Unable to fetch GPG key from keyserver

2xsec · 2018-06-30T10:31:29Z

@brauner ok.

brauner

Basically just nits and questions. Otherwise lgtm. Once those things are addressed I'll merge.

brauner · 2018-06-30T10:35:08Z

src/lxc/tools/lxc_autostart.c

-	if (! p1 && ! str_ptr)
-		return 1;
-	if (! p1 && ! *str_ptr)
+	if (!p1 && !str_ptr)


If you're trying to replace the logic exactly shouldn't this be:

if (!p1 && !str_ptr || !p1 && !*str_ptr)

or

if (!p1 && (!str_ptr || !*str_ptr))

?

Yes, you're right. I fixed it.

brauner · 2018-06-30T10:35:58Z

src/lxc/tools/lxc_autostart.c

-			if (strcmp(it1->elem, it2->elem) == 0)
-				return 1;
-		}
+		if (!strncmp(it1->elem, str_ptr, strlen(it1->elem)))


Please use strncmp() == 0 :)

I fixed it.

brauner · 2018-06-30T10:37:11Z

src/lxc/tools/lxc_autostart.c

@@ -372,7 +361,7 @@ int main(int argc, char *argv[])
 	qsort(&containers[0], count, sizeof(struct lxc_container *), cmporder);

 	if (cmd_groups_list && my_args.all)
-		fprintf(stderr, "Specifying -a (all) with -g (groups) doesn't make sense. All option overrides.\n");
+		ERROR("Specifying -a (all) with -g (groups) doesn't make sense. All option overrides.");


Please remove the trailing . while you're at it. :)

I fixed it.

brauner · 2018-06-30T10:37:29Z

src/lxc/tools/lxc_autostart.c

@@ -404,27 +393,28 @@ int main(int argc, char *argv[])
 				/* We're done with this container */
 				if ( lxc_container_put(c) > 0 )


You can remove the unnecessary spaces here as well. :)

I fixed it.

brauner · 2018-06-30T10:38:55Z

src/lxc/tools/lxc_config.c

@@ -60,21 +61,25 @@ int main(int argc, char *argv[])
 	struct lxc_config_items *i;
 	const char *value;

-	if (argc < 2 || strcmp(argv[1], "-h") == 0 ||
-			strcmp(argv[1], "--help") == 0)
+	if (argc < 2 || !strncmp(argv[1], "-h", strlen(argv[1])) ||


== 0 syntax, please. It's in our coding style. :)

I fixed it.

brauner · 2018-06-30T10:41:12Z

src/lxc/tools/lxc_create.c

+			if (strncmp(a->bdevtype, "lvm", strlen(a->bdevtype)) &&
+			    strncmp(a->bdevtype, "loop", strlen(a->bdevtype)) &&
+			    strncmp(a->bdevtype, "rbd", strlen(a->bdevtype))) {
+				ERROR("Filesystem type and size are only valid with block devices");


!= 0 syntax, please :)

I fixed it.

brauner · 2018-06-30T10:42:04Z

src/lxc/tools/lxc_create.c

 	if (!my_args.bdevtype)
 		my_args.bdevtype = "_unset";

 	if (!validate_bdev_args(&my_args))
 		exit(EXIT_FAILURE);

-	if (strcmp(my_args.bdevtype, "none") == 0)
+	if (!strncmp(my_args.bdevtype, "none", strlen(my_args.bdevtype)))


I'm note sure those are necessary, strcmp() should be sufficient and safe and spares us a strlen() call.

We are using strncmp instead of strcmp to prevent bof attack habitually. But input validation and checking length for input are valid, there is no problem. In case of this, it is sufficient and safe. You are right.

brauner · 2018-06-30T10:42:54Z

src/lxc/tools/lxc_device.c

 	if (p < 0) {
-		fprintf(stderr, "failed to fork task.\n");
+		ERROR("Failed to fork task.");


Ah, can you remove the trailing . as well :)

brauner · 2018-06-30T10:45:27Z

src/lxc/tools/lxc_ls.c

@@ -1290,3 +1323,51 @@ static void ls_field_width(const struct ls *l, const size_t size,
 		}
 	}
 }
+
+static int rm_r(char *dirname)


Hm, you could also move static int recursive_destroy(char *dirname) from cgroups/cgfsng.c to utils.{c,h} and call it rm_r() and use it in both places.

I'll check it you mentioned after tools are merged.

brauner · 2018-06-30T10:46:08Z

src/lxc/tools/lxc_snapshot.c

@@ -218,13 +221,13 @@ static int do_snapshot_destroy(struct lxc_container *c, char *snapname)
 {
 	bool ret;

-	if (strcmp(snapname, "ALL") == 0)
+	if (!strncmp(snapname, "ALL", strlen(snapname)))


== 0 syntax, please

I fixed it.

…ncmp Signed-off-by: 2xsec <dh48.jeong@samsung.com>

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

brauner · 2018-07-01T17:23:04Z

jenkins: test this please

brauner · 2018-07-01T17:38:03Z

Thanks! :)

2xsec · 2018-07-02T05:36:54Z

Thanks for your review.

2xsec added 7 commits June 28, 2018 22:49

tools: lxc-start: share internal API symbols

eb0c938

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-stop: share internal API symbols

19eacdc

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-freeze: share internal API symbols

a599e9c

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-unfreeze: share internal API symbols

7d6ee13

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-wait: share internal API symbols

df3ab00

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-cgroup: share internal API symbols

93f81bc

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-attach: share internal API symbols

1b36d9e

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

2xsec added 9 commits June 30, 2018 18:45

tools: lxc-autostart: share internal API symbols

1cd7db6

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

utils: code cleanups

b14fc10

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-info: share internal API symbols

469711c

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-ls: share internal API symbols

d2ba12a

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-config: share internal API symbols

b27cdf6

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-console: share internal API symbols

138eda9

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-destroy: share internal API symbols

75e607b

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-device: share internal API symbols

a9cb0fb

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-execute: share internal API symbols

d899f11

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

2xsec added 8 commits June 30, 2018 18:45

tools: lxc-unshare: share internal API symbols

4f66541

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-checkpoint: share internal API symbols

124fef5

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-checkpoint: fix log name

4888c73

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-snapshot: share internal API symbols

ce9ed7c

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-create: share internal API symbols

9c38292

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-monitor: share internal API symbols

457e3c5

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: arguments: share internal API symbols

ae41d18

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: remote tool_list, tool_utils

03e6adc

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

brauner requested changes Jun 30, 2018

View reviewed changes

2xsec added 11 commits July 1, 2018 22:37

tools: lxc-autostart: remove the trailing . & fix coding style of str…

54d4713

…ncmp Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-checkpoint: remove the trailing .

8fb41a3

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-config: fix coding style of strncmp

dc71fcc

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-copy: remove the trailing . & fix coding style of strncmp

5dec9c3

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-create: remove the trailing . & fix coding style of strncmp

77e5da8

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-destroy: remove the trailing .

8f0bdb0

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-device: remove the trailing . & fix coding style of strncmp

80287d7

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-snapshot: fix coding style of strncmp

d5202dd

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-start: remove the trailing .

28b7b0f

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-top: fix coding style of strncmp

162ad1f

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

tools: lxc-wait: remove the trailing .

1b087da

Signed-off-by: 2xsec <dh48.jeong@samsung.com>

brauner merged commit 2d876a9 into lxc:master Jul 1, 2018

		@@ -404,27 +393,28 @@ int main(int argc, char *argv[])
		/* We're done with this container */
		if ( lxc_container_put(c) > 0 )

tools: share internal API symbols #2437

tools: share internal API symbols #2437

Conversation

2xsec commented Jun 28, 2018

lxc-jenkins commented Jun 28, 2018

brauner commented Jun 28, 2018

brauner commented Jun 28, 2018

brauner commented Jun 29, 2018

2xsec commented Jun 29, 2018

brauner commented Jun 29, 2018

brauner commented Jun 29, 2018

2xsec commented Jun 29, 2018

brauner commented Jun 29, 2018

brauner commented Jun 29, 2018

2xsec commented Jun 30, 2018 • edited Loading

brauner commented Jun 30, 2018 • edited Loading

2xsec commented Jun 30, 2018

brauner commented Jun 30, 2018

2xsec commented Jun 30, 2018 • edited Loading

brauner commented Jun 30, 2018

2xsec commented Jun 30, 2018

2xsec commented Jun 30, 2018

brauner commented Jun 30, 2018

2xsec commented Jun 30, 2018

brauner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brauner commented Jul 1, 2018

brauner commented Jul 1, 2018

2xsec commented Jul 2, 2018

2xsec commented Jun 30, 2018 •

edited

Loading

brauner commented Jun 30, 2018 •

edited

Loading

2xsec commented Jun 30, 2018 •

edited

Loading