Hi Nikiforov,
Thanks for taking the time to send a patch through. Some comments
(more related to the styles as opposed to the patch itself)
On Thu, Jul 7, 2011 at 1:26 AM, Nikiforov Alex <a.nikifo...@samsung.com> wrote:
> From ed3f8adf640080ecae89edf27c7283e62c41beb0 Mon Sep 17 00:00:00 2001
> From: Alex Nikiforov <a.nikifo...@samsung.com>
> Date: Mon, 4 Jul 2011 15:17:38 +0400
> Subject: [PATCH 1/2] cgrulesengd: cgclassify: fix read/write with vector
> operation readv/writev, move PID check from server to client, if PID is
> not valid we dont need any read/write
>
Can we please have the subject in the subject? Also, this description
is a bit confused. More importantly, it gives me the impression that
more than one thing is being done in this patch. I would prefer if
each item had its own patch. So, it would be
1. Fix read/write with vector operations
2. Move PID check from server to client
3. Check PID validity (?)
Finally, it would be great to give some sort of a justification for
the change (unless it is obvious from the subject).
> ---
> src/api.c | 15 ++++++++++++---
> src/daemon/cgrulesengd.c | 23 +++++++++++------------
> src/tools/cgclassify.c | 8 ++++++--
> 3 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/src/api.c b/src/api.c
> index d5022c2..01b0fb2 100644
> --- a/src/api.c
> +++ b/src/api.c
> @@ -43,6 +43,7 @@
> #include <sys/socket.h>
> #include <fcntl.h>
> #include <sys/syscall.h>
> +#include <sys/uio.h>
> #include <unistd.h>
> #include <fts.h>
> #include <ctype.h>
> @@ -3704,6 +3705,7 @@ int cgroup_register_unchanged_process(pid_t pid,
> int flags)
> int ret = 1;
> char buff[sizeof(CGRULE_SUCCESS_STORE_PID)];
> struct sockaddr_un addr;
> + struct iovec iov[2] = {}; /* [pid] [flags] */
>
this comment does not help here.
> sk = socket(PF_UNIX, SOCK_STREAM, 0);
> if (sk < 0)
> @@ -3720,11 +3722,18 @@ int cgroup_register_unchanged_process(pid_t pid,
> int flags)
> ret = 0;
> goto close;
> }
> - if (write(sk, &pid, sizeof(pid)) < 0)
> - goto close;
>
> - if (write(sk, &flags, sizeof(flags)) < 0)
> + /* FIXME - change for writev() */
> + /* prepare vectors for reading */
> + iov[0].iov_base = &pid;
> + iov[0].iov_len = sizeof(pid);
> + iov[1].iov_base = &flags;
> + iov[1].iov_len = sizeof(flags);
> +
> + if( writev(sk, iov, 2) < 0 ) {
> + //cgroup_dbg("read error: %s\n", strerror(errno));
Remove this comment (also wrong style comments)
> goto close;
> + }
>
> if (read(sk, buff, sizeof(buff)) < 0)
> goto close;
> diff --git a/src/daemon/cgrulesengd.c b/src/daemon/cgrulesengd.c
> index 2f42a57..42d15f1 100644
> --- a/src/daemon/cgrulesengd.c
> +++ b/src/daemon/cgrulesengd.c
> @@ -40,6 +40,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/socket.h>
> +#include <sys/uio.h>
> #include <sys/syslog.h>
> #include <string.h>
> #include <linux/netlink.h>
> @@ -550,8 +551,7 @@ static void cgre_receive_unix_domain_msg(int sk_unix)
> pid_t pid;
> struct sockaddr_un caddr;
> socklen_t caddr_len;
> - struct stat buff_stat;
> - char path[FILENAME_MAX];
> + struct iovec iov[2] = {}; /* [pid] [flags] */
>
> caddr_len = sizeof(caddr);
> fd_client = accept(sk_unix, (struct sockaddr *)&caddr, &caddr_len);
> @@ -559,19 +559,18 @@ static void cgre_receive_unix_domain_msg(int sk_unix)
> cgroup_dbg("accept error: %s\n", strerror(errno));
> return;
> }
> - if (read(fd_client, &pid, sizeof(pid)) < 0) {
> - cgroup_dbg("read error: %s\n", strerror(errno));
> - goto close;
> - }
> - sprintf(path, "/proc/%d", pid);
> - if (stat(path, &buff_stat)) {
> - cgroup_dbg("There is not such process (PID: %d)", pid);
> - goto close;
> - }
> - if (read(fd_client, &flags, sizeof(flags)) < 0) {
> +
> + /* prepare vectors for reading */
> + iov[0].iov_base = &pid;
> + iov[0].iov_len = sizeof(pid);
> + iov[1].iov_base = &flags;
> + iov[1].iov_len = sizeof(flags);
> +
> + if( readv(fd_client, iov, 2) < 0 ) {
> cgroup_dbg("read error: %s\n", strerror(errno));
> goto close;
> }
> +
> if (flags == CGROUP_DAEMON_CANCEL_UNCHANGE_PROCESS) {
> cgre_remove_unchanged_process(pid);
> } else {
> diff --git a/src/tools/cgclassify.c b/src/tools/cgclassify.c
> index 397b725..f103667 100644
> --- a/src/tools/cgclassify.c
> +++ b/src/tools/cgclassify.c
> @@ -121,7 +121,8 @@ int main(int argc, char *argv[])
> struct cgroup_group_spec *cgroup_list[CG_HIER_MAX];
> int c;
> char *endptr;
> -
> + char path[FILENAME_MAX];
> + struct stat buff_stat;
>
> if (argc < 2) {
> usage(1, argv[0]);
> @@ -168,7 +169,10 @@ int main(int argc, char *argv[])
>
> for (i = optind; i < argc; i++) {
> pid = (uid_t) strtol(argv[i], &endptr, 10);
> - if (endptr[0] != '\0') {
> +
> + /* is it correct and valid PID ? */
> + snprintf(path, FILENAME_MAX,"/proc/%d", pid);
> + if (stat(path, &buff_stat)) {
> /* the input argument was not a number */
> fprintf(stderr, "Error: %s is not valid pid.\n",
> argv[i]);
> --
------------------------------------------------------------------------------
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
