ps: ps command implementation by devimc · Pull Request #30 · clearcontainers/hyperstart

devimc · 2017-05-23T21:00:03Z

this patch adds the implementation for ps command, ps output will
be parsed to see what processes are running inside the given
container therefore a list of processes that belong to the container
is returned

Signed-off-by: Julio Montes julio.montes@intel.com

dlespiau · 2017-05-23T21:46:08Z

Not sure it's a good idea to keep adding features here. We're happy to have feature-parity with 2.1 as far as hyperstart is concerned for 3.0.

devimc · 2017-05-24T12:49:56Z

@dlespiau what do you mean? should I add this new feature in master? I thought cc 3.0 was using hyperstart 0.7.0

dlespiau · 2017-05-24T13:29:04Z

I hadn't noticed samuel had added docker ps support in the beta project. I guess we need that. The point is we should try to limit new features: we're not properly upstreaming - which creates a lot of technical debt - and we'd like to use our own agent at some point.

jodh-intel · 2017-05-30T16:35:23Z

src/init.c

+	/**
+	 * line to process, it contains information about a process
+	 * for example:
+	 *   root 5123 2 0 09:51 ? 00:00:00 [kworker/2:0]


I like the example, but since this is at the top of the file, it would be good to explain that the format will vary, depending on the value of psargs.

jodh-intel · 2017-05-30T16:41:53Z

src/init.c

+	char **array = NULL;
+	char *new_line = NULL;
+	int ret = -1;
+	const char* pid_str = "PID";


I'm wondering if it might be better to:

Require psargs to specify the output format such that the first field must be the pid.

If no psargs are specified, make hyperstart specify the format in the same way:
ps -o pid,ppid,pgrp,session,tpgid,tty,comm -e or something).

The advantage of doing that:

You could remove pid_index from hyper_ps_data.

It would be easy to reject the PSCONTAINER command if the first field wasn't purely numeric on every line.

You wouldn't need this function.

The code would be safer since what happens if psargs is just --no-headers at the moment?

it is a good idea indeed, but -o option will override all other options, for example:

$ ps -ef UID PID PPID C STIME TTY TIME CMD

ps -o pid,ppid,pgrp,session,tpgid,tty -ef PID PPID PGRP SESS TPGID TT

jodh-intel · 2017-05-30T16:46:11Z

src/init.c

+	/* +1 for \n */
+	size_t line_len = ps_data->line_len + 1;
+	size_t output_len = *ps_data->output_len;
+	const size_t buf_size = 1024;


This limit might be too low: on a test box, I've got a qemu cmdline that is 2634 bytes long.

1024 is the initial size for the buffer, the size will increase by 1024 in each successive realloc

Of course. Might be a good idea to put a comment above it for clarity (or even call it initial_buf_size or buf_size_increment maybe).

dlespiau · 2017-05-31T10:31:46Z

wouldn't it be possible/better to do an "exec" with the ps command instead of introducing a new command that doesn't seem to add value compared to an exec? what am I missing?

dlespiau · 2017-05-31T10:40:55Z

Ah, answering to myself. We don't necessarily have ps in the container rootfs. This adds a new dependency for the guest OS though.

devimc · 2017-05-31T13:00:53Z

@dlespiau yes, is not a guarantee that container rootfs will have the ps command

jodh-intel · 2017-05-31T13:05:10Z

src/init.c

+ *
+ * \return pid index on success, else -1
+ */
+static int hyper_get_pid_index(const char *line)


Missing a check on line being NULL.

fixing, thanks

jodh-intel · 2017-05-31T13:05:32Z

src/init.c

+ *
+ * \return true on success, else false
+ */
+static bool hyper_append_ps_line(struct hyper_ps_data *ps_data)


Check ps_data against NULL before dereferencing.

jodh-intel · 2017-05-31T13:15:08Z

src/init.c

+ *
+ * \return true on success, else false
+ */
+static bool hyper_process_ps_line(struct hyper_ps_data *ps_data)


Missing check on ps_data.

jodh-intel · 2017-05-31T13:16:01Z

src/init.c

+	array = hyper_line_to_array(new_process_line, &array_size, ps_data->pid_index+1);
+	if (array == NULL) {
+		free_if_set(new_process_line);
+		fprintf(stderr, "can not convert line to array: %s\n", new_process_line);


The fprintf() needs to be before the free_if_set().

good catch!
thanks

jodh-intel · 2017-05-31T13:20:45Z

src/init.c

+	}
+
+	/* processes less than container pid are not part of the container */
+	if (pid < ps_data->container_pid) {


Is this test reliable I wonder? The container_pid is in a separate namespace so might be 1 inside the container and 999 outside it in the VM context.

What if the container creates lots of processes and reaches the limit of a pid_t. Will the next pid be guaranteed to be > container_pid (>999 in the VM context in this example)?

you're right it is not a valid condition once limit is reached it is restarted

jodh-intel · 2017-05-31T13:23:58Z

src/init.c

+ *
+ * \return 0 on success, else -1
+ */
+static int hyper_ps_container(char *json, int length, uint32_t *datalen, uint8_t **data)


Args need checks.

jodh-intel · 2017-05-31T13:25:51Z

src/util.c

+	size_t bufsize = 512;
+	uint8_t *buffer = NULL;
+
+	buffer = calloc(bufsize, sizeof(*buffer));


What if calloc() fails?

adding validation, thanks

jodh-intel · 2017-05-31T13:27:07Z

src/util.c

+	buffer = calloc(bufsize, sizeof(*buffer));
+
+	do {
+		bytes_read = read(stdout_pipe,


The tests in this block do seem safe, but I'd still suggest checking if bytes_read < 0 and handling that immediately rather than implicitly.

jodh-intel · 2017-05-31T13:28:55Z

src/util.c

+		}
+		if (total_bytes_read >= bufsize-1) {
+			uint8_t *tmp = NULL;
+			bufsize+=bufsize;


Just a check: do you intend to double the size (I suspect so), or do you want to add 512?

I want to double the size

jodh-intel · 2017-05-31T13:29:33Z

src/util.c

+ *
+ * \return 0 on success, else -1
+ */
+static int hyper_read_cmd_stdout(int stdout_pipe, uint8_t **output, size_t *size)


Missing checks on stdout_pipe and output params.

amshinde · 2017-06-01T21:13:12Z

src/util.c

+
+	if (buffer == NULL) {
+		/* try once again */
+		buffer = calloc(bufsize, sizeof(*buffer));


If calloc fails, it usually indicates OOM. No point trying again.

I prefer to keep it, if OOM occurs hyperstart will fail any way

amshinde · 2017-06-01T21:19:48Z

src/util.c

+
+		if (bytes_read > 0) {
+			total_bytes_read += bytes_read;
+			buffer[total_bytes_read] = '\0';


This assignment can be moved out of the loop I think to perform it just once

yes, thank you!

amshinde · 2017-06-01T21:21:41Z

src/util.c

+			buffer[total_bytes_read] = '\0';
+		}
+
+		if (total_bytes_read >= bufsize-1) {


Does this have to be > rather than >= check here?

this patch adds two extra parameters to hyper_cmd for saving command stdout and a new function to read and save the data from command stdout fd Signed-off-by: Julio Montes <julio.montes@intel.com>

…er_cmd Signed-off-by: Julio Montes <julio.montes@intel.com>

this patch adds the implementation for ps command, ps output will be parsed to see what processes are running inside the given container therefore a list of processes that belong to the container is returned Signed-off-by: Julio Montes <julio.montes@intel.com>

egernst · 2019-02-08T17:06:14Z

can we close this? :)

devimc requested a review from amshinde May 23, 2017 21:00

devimc added do-not-merge WIP labels May 23, 2017

devimc mentioned this pull request May 29, 2017

agent: implement ps command containers/virtcontainers#269

Merged

devimc force-pushed the topic/psCommand branch 2 times, most recently from 96a3f0e to 46b1878 Compare May 30, 2017 15:15

devimc changed the title ~~[WIP] ps: ps command implementation~~ ps: ps command implementation May 30, 2017

devimc removed do-not-merge WIP labels May 30, 2017

devimc requested a review from jodh-intel May 30, 2017 15:16

jodh-intel reviewed May 30, 2017

View reviewed changes

jodh-intel reviewed May 31, 2017

View reviewed changes

devimc force-pushed the topic/psCommand branch 3 times, most recently from 00d77b5 to ea192fb Compare May 31, 2017 16:07

amshinde reviewed Jun 1, 2017

View reviewed changes

Julio Montes added 3 commits June 5, 2017 14:54

util: add extra parameters to hyper_cmd for saving command stdout

d8a18a0

this patch adds two extra parameters to hyper_cmd for saving command stdout and a new function to read and save the data from command stdout fd Signed-off-by: Julio Montes <julio.montes@intel.com>

portmapping: fix portmapping according with new implementation of hyp…

854d97b

…er_cmd Signed-off-by: Julio Montes <julio.montes@intel.com>

ps: implements ps command

6f73ee7

this patch adds the implementation for ps command, ps output will be parsed to see what processes are running inside the given container therefore a list of processes that belong to the container is returned Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the topic/psCommand branch from ea192fb to 6f73ee7 Compare June 5, 2017 19:55

egernst closed this Feb 8, 2019

Conversation

devimc commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlespiau commented May 23, 2017

Uh oh!

devimc commented May 24, 2017

Uh oh!

dlespiau commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlespiau commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlespiau commented May 31, 2017

Uh oh!

devimc commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

devimc commented May 23, 2017 •

edited

Loading

dlespiau commented May 31, 2017 •

edited

Loading