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.

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

Add process iterator test #131

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add process iterator test #131

wants to merge 4 commits into from

Conversation

Tacklebox
Copy link

No description provided.

@Tacklebox Tacklebox requested a review from haesbaert March 13, 2025 16:35
@Tacklebox Tacklebox requested a review from a team as a code owner March 13, 2025 16:35
Comment on lines +458 to +461
for (i = 0; i < n_children; i++) {
children[i] = fork_exec_nop();
drain_for_pid(&qq, children[i]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (i = 0; i < n_children; i++) {
children[i] = fork_exec_nop();
drain_for_pid(&qq, children[i]);
}
for (i = 0; i < (int)nitems(children); i++)
children[i] = fork_exec_nop();
for (i = 0; i < (int)nitems(children); i++)
children[i] = drain_for_pid(&qq, children[i]);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Small trick, use nitems() so you can cut n_children out.
Because of how aging works in quark, it makes more sense to fork all + drain all, otherwise each drain will have to age the event for 25ms, so this cuts 75ms->25ms

Comment on lines +445 to +446
int n_children = 3;
pid_t children[n_children];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int n_children = 3;
pid_t children[n_children];
pid_t children[3];

See the nitems() comment below, also .

Comment on lines +456 to +457
quark_process_iter_init(&qi, &qq);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you initialize the iterator before drain_for_pid, so the iterator can be invalidated, just move this to before the main loop. I realize this is not ideal, maybe I should change iter_init to point to NULL, instead of pointing to RB_ROOT.

Comment on lines +467 to +529
assert(qp->flags & QUARK_F_EXIT);
assert(qp->flags & QUARK_F_COMM);
assert(qp->flags & QUARK_F_FILENAME);
assert(qp->flags & QUARK_F_CMDLINE);
assert(qp->flags & QUARK_F_CWD);
assert((pid_t)qp->proc_ppid == getpid());
assert(qp->proc_time_boot > 0); /* XXX: improve */
assert(qp->proc_uid == getuid());
assert(qp->proc_gid == getgid());
assert(qp->proc_suid == geteuid());
assert(qp->proc_sgid == getegid());
assert(qp->proc_euid == geteuid());
assert(qp->proc_egid == getegid());
assert((pid_t)qp->proc_pgid == getpgid(0));
assert((pid_t)qp->proc_sid == getsid(0));
/* check capabilities */
/* XXX assumes we're root */
assert(qp->proc_cap_inheritable == 0);
/*
* We don't know the exact set since it varies from kernel,
* improve this in the future.
*/
assert(qp->proc_cap_effective != 0);
assert(qp->proc_cap_permitted != 0);
/* check entry leader */
/*
* XXX TODO This depends how we're running the test, if we're over ssh
* it will show ssh, if not it will show init and whatnot, for now
* assert that it is not unknown at least.
*/
assert(qp->proc_entry_leader != 0);
assert(qp->proc_entry_leader_type != QUARK_ELT_UNKNOWN);
/* XXX TODO check tty_major and tty_minor for self in the future */
#if 0
assert(qp->proc_tty_major != QUARK_TTY_UNKNOWN);
assert(qp->proc_tty_minor != 0);
#endif
/* check strings */
assert(!strcmp(qp->comm, program_invocation_short_name));
assert(!strcmp(qp->filename, binpath()));
/* check args */
args = args_make(qp);
assert(args != NULL);
assert(args->argc == 5);
assert(!strcmp(args->argv[0], binpath()));
assert(!strcmp(args->argv[1], "-N"));
assert(!strcmp(args->argv[2], "this"));
assert(!strcmp(args->argv[3], "is"));
assert(!strcmp(args->argv[4], "nop!"));
/*
* Expected len is the length of the arguments summed up, plus one byte
* for each argument(the NUL after each argument, including the last
* one), so we just start at 'argc' bytes.
*/
for (expected_len = args->argc, j = 0; j < args->argc; j++)
expected_len += strlen(args->argv[j]);
args_free(args);
assert(qp->cmdline_len == expected_len);

if (getcwd(cwd, sizeof(cwd)) == NULL)
err(1, "getcwd");
assert(!strcmp(cwd, qp->cwd));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert(qp->flags & QUARK_F_EXIT);
assert(qp->flags & QUARK_F_COMM);
assert(qp->flags & QUARK_F_FILENAME);
assert(qp->flags & QUARK_F_CMDLINE);
assert(qp->flags & QUARK_F_CWD);
assert((pid_t)qp->proc_ppid == getpid());
assert(qp->proc_time_boot > 0); /* XXX: improve */
assert(qp->proc_uid == getuid());
assert(qp->proc_gid == getgid());
assert(qp->proc_suid == geteuid());
assert(qp->proc_sgid == getegid());
assert(qp->proc_euid == geteuid());
assert(qp->proc_egid == getegid());
assert((pid_t)qp->proc_pgid == getpgid(0));
assert((pid_t)qp->proc_sid == getsid(0));
/* check capabilities */
/* XXX assumes we're root */
assert(qp->proc_cap_inheritable == 0);
/*
* We don't know the exact set since it varies from kernel,
* improve this in the future.
*/
assert(qp->proc_cap_effective != 0);
assert(qp->proc_cap_permitted != 0);
/* check entry leader */
/*
* XXX TODO This depends how we're running the test, if we're over ssh
* it will show ssh, if not it will show init and whatnot, for now
* assert that it is not unknown at least.
*/
assert(qp->proc_entry_leader != 0);
assert(qp->proc_entry_leader_type != QUARK_ELT_UNKNOWN);
/* XXX TODO check tty_major and tty_minor for self in the future */
#if 0
assert(qp->proc_tty_major != QUARK_TTY_UNKNOWN);
assert(qp->proc_tty_minor != 0);
#endif
/* check strings */
assert(!strcmp(qp->comm, program_invocation_short_name));
assert(!strcmp(qp->filename, binpath()));
/* check args */
args = args_make(qp);
assert(args != NULL);
assert(args->argc == 5);
assert(!strcmp(args->argv[0], binpath()));
assert(!strcmp(args->argv[1], "-N"));
assert(!strcmp(args->argv[2], "this"));
assert(!strcmp(args->argv[3], "is"));
assert(!strcmp(args->argv[4], "nop!"));
/*
* Expected len is the length of the arguments summed up, plus one byte
* for each argument(the NUL after each argument, including the last
* one), so we just start at 'argc' bytes.
*/
for (expected_len = args->argc, j = 0; j < args->argc; j++)
expected_len += strlen(args->argv[j]);
args_free(args);
assert(qp->cmdline_len == expected_len);
if (getcwd(cwd, sizeof(cwd)) == NULL)
err(1, "getcwd");
assert(!strcmp(cwd, qp->cwd));

I think we can cut this, this is already tested multiple times in other places, if we find the processes through iteration I'm happy.

Comment on lines +463 to +466
for (i = 0; i < n_children; i++) {
do {
qp = quark_process_iter_next(&qi);
} while (qp != NULL && (pid_t)qp->pid != children[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The outer loop should be the iterator, and then look for the children.
This is only working now because of how the internal tree is sorted, if that changes, you would be skipping elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants