Re: [systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2013-01-16 Thread Zbigniew Jędrzejewski-Szmek 
On Wed, Jan 16, 2013 at 04:59:47PM +0100, Lukáš Nykrýn wrote:
 Subject: [PATCH] systemctl,loginctl,cgls: do not ellipsize cgroup members
  when --full is specified
 
 New file output.h with output flags and modes.
 
 --full parameter also for cgls and loginctl.
 
 Include 'all' parameter in flags (show_cgroup_by_path, show_cgroup,
 show_cgroup_and_extra, show_cgroup_and_extra_by_spec).
 
 get_process_cmdline with max_length == 0 will not ellipsize output.
 
 Replace LINE_MAX with 0 in some calls of get_process_cmdline.
Hi,
I commited your patch with some small changes. Most importantly, I
dropped '-f'. You actually forgot to add 'f' to the getopt invocation,
so it didn't work. I took that to mean that the option is not quite
necessary :) Seriously speaking, I also changed cgls to default to
--full, so the short option is likely not necessary. If we add a short
option for --full, I think that we should find a different letter ('U'
seems to be the only one free), which can be shared between different
systemd commands. I don't want '-f' to mean '--full' sometimes, and
'--force' othertimes, leading to unexpected results.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2013-01-15 Thread Lukáš Nykrýn 
Lennart Poettering píše v Út 25. 09. 2012 v 17:45 +0200:
 On Tue, 25.09.12 15:36, Lukáš Nykrýn (lnyk...@redhat.com) wrote:
 
 Heya,
 
  diff --git a/src/shared/logs-show.h b/src/shared/logs-show.h
  index 3e6b6e0..3b63a5d 100644
  --- a/src/shared/logs-show.h
  +++ b/src/shared/logs-show.h
  @@ -27,26 +27,6 @@
   
   #include util.h
   
  -typedef enum OutputMode {
  -OUTPUT_SHORT,
  -OUTPUT_SHORT_MONOTONIC,
  -OUTPUT_VERBOSE,
  -OUTPUT_EXPORT,
  -OUTPUT_JSON,
  -OUTPUT_JSON_PRETTY,
  -OUTPUT_CAT,
  -_OUTPUT_MODE_MAX,
  -_OUTPUT_MODE_INVALID = -1
  -} OutputMode;
  -
  -typedef enum OutputFlags {
  -OUTPUT_SHOW_ALL   = 1  0,
  -OUTPUT_FOLLOW = 1  1,
  -OUTPUT_WARN_CUTOFF= 1  2,
  -OUTPUT_FULL_WIDTH = 1  3,
  -OUTPUT_COLOR  = 1  4
  -} OutputFlags;
  -
 
 Hmm, I don't think this should be part of util.[ch] really, it's far to
 specific to be considered just a utility. Could you please move this to a new
 file output.h or so?
 
  -int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, 
  char **line) {
  -char *r, *k;
  +int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, 
  char **line, OutputFlags flags) {
  +char *r = NULL, *k;
   int c;
  -bool space = false;
  -size_t left;
   FILE *f;
 
 I don't think this flag really should be passed here, this call is too
 low-level for that. Instead, max_length == 0 could be used as a good
 indicator for as big as needed or so.
 
 Otherwise looks pretty OK!
 
 Lennart
 

Hello,
I completely forget about this patch and there were again some
complaints about this behavior. Here is my original patch with some
modification (see commit log).

Lukas


From 88495e0e6a3fb9ff95a28a65c22b3b55e21c14fa Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 14 Jan 2013 18:16:50 +0100
Subject: [PATCH] systemctl,loginctl,cgls: do not ellipsize cgroup members
 when --full is specified

New file output.h with output flags and modes.

--full parameter also for cgls and loginctl.

Include 'all' parameter in flags (show_cgroup_by_path, show_cgroup,
show_cgroup_and_extra, show_cgroup_and_extra_by_spec).

get_process_cmdline with max_length == 0 will not ellipsize output.

Replace LINE_MAX with 0 in some calls of get_process_cmdline.
---
 Makefile.am   |  3 +-
 man/loginctl.xml  |  7 
 man/systemctl.xml |  2 +-
 man/systemd-cgls.xml  |  8 +
 src/cgls/cgls.c   | 16 ++---
 src/core/selinux-access.c |  4 +--
 src/journal/coredump.c|  2 +-
 src/journal/journald-server.c |  2 +-
 src/login/loginctl.c  | 13 +--
 src/shared/cgroup-show.c  | 49 +
 src/shared/cgroup-show.h  | 10 +++---
 src/shared/logs-show.h| 23 +---
 src/shared/output.h   | 44 +++
 src/shared/util.c | 84 +--
 src/systemctl/systemctl.c | 14 
 15 files changed, 178 insertions(+), 103 deletions(-)
 create mode 100644 src/shared/output.h

diff --git a/Makefile.am b/Makefile.am
index 3318829..f4f9f34 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -809,7 +809,8 @@ libsystemd_shared_la_SOURCES = \
 	src/shared/time-dst.c \
 	src/shared/time-dst.h \
 	src/shared/calendarspec.c \
-	src/shared/calendarspec.h
+	src/shared/calendarspec.h \
+	src/shared/output.h
 
 libsystemd_shared_la_LIBADD = libsystemd-daemon.la
 
diff --git a/man/loginctl.xml b/man/loginctl.xml
index 5dbc1f6..8a20d18 100644
--- a/man/loginctl.xml
+++ b/man/loginctl.xml
@@ -110,6 +110,13 @@
 set or not./para/listitem
 /varlistentry
 
+varlistentry
+termoption--full/option/term
+
+listitemparaDo not ellipsize cgroup
+members./para
+/listitem
+/varlistentry
 
 varlistentry
 termoption--no-pager/option/term
diff --git a/man/systemctl.xml b/man/systemctl.xml
index 2f33e0c..0289200 100644
--- a/man/systemctl.xml
+++ b/man/systemctl.xml
@@ -156,7 +156,7 @@
 termoption--full/option/term
 
 listitemparaDo not ellipsize unit
-names and truncate unit descriptions
+names, cgroup members and truncate unit descriptions
 in the output of
 commandlist-units/command and
 commandlist-jobs/command./para/listitem
diff --git a/man/systemd-cgls.xml b/man/systemd-cgls.xml
index 4b6ee93..b280b87 100644
---

Re: [systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2013-01-15 Thread Zbigniew Jędrzejewski-Szmek 
On Tue, Jan 15, 2013 at 10:58:26AM +0100, Lukáš Nykrýn wrote:
 Subject: [PATCH] systemctl,loginctl,cgls: do not ellipsize cgroup members
  when --full is specified
 
 diff --git a/man/systemctl.xml b/man/systemctl.xml
 index 2f33e0c..0289200 100644
 --- a/man/systemctl.xml
 +++ b/man/systemctl.xml
 @@ -156,7 +156,7 @@
  termoption--full/option/term
  
  listitemparaDo not ellipsize unit
 -names and truncate unit descriptions
 +names, cgroup members and truncate unit 
 descriptions
^comma here
  in the output of
  commandlist-units/command and
  
 commandlist-jobs/command./para/listitem
Hi Lukas,
I don't think it's working as advertised:

$  systemd/build/systemctl status -f network.service
 Loaded: loaded (/etc/rc.d/init.d/network)
   Active: active (running) since Mon 2013-01-14 04:00:35 CET; 1 day 13h ago
Process: 476 ExecStart=/etc/rc.d/init.d/network start (code=exited, 
status=0/SUCCESS)
  CGroup: name=systemd:/system/network.service
\661 /sbin/dhclient -H fedora-15 -1 -q -cf /etc/dh...

I'd expect this last line to be shown in full.

Also, unit names seem to always be ellipsized:

$ systemd/build/systemctl -f
UNIT  LOAD   ACTIVE SUB   DESCRIPTION
proc-sys...misc.automount loaded active waiting   Arbitrary Executable F

AFAICT, your patch didn't change this behaviour, but it
would be nice to bring the documentation and behaviour in
line.

 diff --git a/man/systemd-cgls.xml b/man/systemd-cgls.xml
 index 4b6ee93..b280b87 100644
 --- a/man/systemd-cgls.xml
 +++ b/man/systemd-cgls.xml
 @@ -111,6 +111,14 @@
  /varlistentry
  
  varlistentry
 +termoption--full/option/term
 +
 +listitemparaDo not ellipsize cgroup
 +members./para
 +/listitem
 +/varlistentry
 +
 +varlistentry
  termoption-k/option/term
This works, but is off by default. If we are under a pager, it makes
little sense to ellipsize long lines, so the default could be reversed.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2013-01-15 Thread Lennart Poettering 
On Tue, 15.01.13 10:58, Lukáš Nykrýn (lnyk...@redhat.com) wrote:

Heya,

 diff --git a/src/shared/output.h b/src/shared/output.h
 new file mode 100644
 index 000..0efd430
 --- /dev/null
 +++ b/src/shared/output.h

output.h is a bit too generic for my test, maybe a better name would
be output-mode.h or so?

  if (pid == 0)
 @@ -919,7 +916,7 @@ int get_process_cmdline(pid_t pid, size_t max_length, 
 bool comm_fallback, char *
  else {
  char *p;
  if (asprintf(p, /proc/%lu/cmdline, (unsigned long) pid)  
 0)
 -return -ENOMEM;
 +return log_oom();

Library calls shouldn't log on their own, except on debug level. Only
app code should. The line between library and app in systemd is a
blurry one as we have everything in the same repo, and link things
together in varying combinations, but util.c is definitely of the
library category and should hence not log errors directly, not even
for OOM, and just return ENOMEM;

  
  f = fopen(p, re);
  free(p);
 @@ -927,47 +924,66 @@ int get_process_cmdline(pid_t pid, size_t max_length, 
 bool comm_fallback, char *
  
  if (!f)
  return -errno;
 +if (max_length == 0) {
 +size_t len = 0, alloc = 0;
 +while ((c = getc(f)) != EOF) {
 +if(alloc = len+1) {
 +alloc += 20;
 +k = realloc(r, alloc);

As I learnt recently realloc() is actually kinda smart internally, and
will increase the allocated buffer in chunks anyway, so that jumping 20
bytes at a time here (or even doing exponential increases as a lot of
code) is actually pretty much unnecessary, and we could just call
realloc() here always with the byte-exact length we need. Doesn't really
make anything slower or faster, but certainly makes thing simpler to read...

 +if (k == NULL) {
 +free(r);
 +fclose(f);
 +return log_oom();

No log_oom() here either, please

 +}
 +r = k;
 +}
 +r[len] = isprint(c) ? c : ' ';
 +r[++len] = 0;
 +}
 +} else {
 +bool space = false;
 +size_t left;
 +r = new(char, max_length);
 +if (!r) {
 +fclose(f);
 +return log_oom();
 +}

Otherwise looks good!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2012-09-25 Thread Lukáš Nykrýn 
Lennart Poettering píše v Pá 21. 09. 2012 v 12:22 +0200:
 On Thu, 20.09.12 16:30, Lukáš Nykrýn (lnyk...@redhat.com) wrote:
 
  Hello,
  From bug report: https://bugzilla.redhat.com/show_bug.cgi?id=858693
  There is no easy way how to force systemctl status not to crop long
  lines with ellipsis in a virtual terminal. -a or --full options
  doesn't help (and they even shouldn't help, according to the man page)
  
  I am not sure if there should be additional parameter or extend --full,
  but systemctl has already enough parameters.
 
 Using --full for this sounds like a good idea.
 
 Insted of using UINT_MAX here as special value I'd prefer if we could
 follow the semantics of logs-show.h here more closely and introduce a
 bit field?
 
 Lennart
 

I think that in this case we can use the OutputFlags so here is reworked
patch.

Regards
Lukas
From 4d5427101094a29a125176d0010842b3093184fb Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Tue, 25 Sep 2012 15:30:03 +0200
Subject: [PATCH] systemctl: do not ellipsize cgroup members in full status

---
 man/systemctl.xml |2 +-
 src/cgls/cgls.c   |6 ++--
 src/core/selinux-access.c |4 +-
 src/journal/coredump.c|2 +-
 src/journal/journald.c|2 +-
 src/login/loginctl.c  |4 +-
 src/shared/cgroup-show.c  |   36 ++--
 src/shared/cgroup-show.h  |9 +++--
 src/shared/logs-show.h|   20 ---
 src/shared/util.c |   81 +++--
 src/shared/util.h |   22 -
 src/systemctl/systemctl.c |   14 
 12 files changed, 110 insertions(+), 92 deletions(-)

diff --git a/man/systemctl.xml b/man/systemctl.xml
index fedc588..eacd2ed 100644
--- a/man/systemctl.xml
+++ b/man/systemctl.xml
@@ -151,7 +151,7 @@
 termoption--full/option/term
 
 listitemparaDo not ellipsize unit
-names and truncate unit descriptions
+names, cgroup members and truncate unit descriptions
 in the output of
 commandlist-units/command and
 commandlist-jobs/command./para/listitem
diff --git a/src/cgls/cgls.c b/src/cgls/cgls.c
index b2cd968..c7c368d 100644
--- a/src/cgls/cgls.c
+++ b/src/cgls/cgls.c
@@ -132,7 +132,7 @@ int main(int argc, char *argv[]) {
 int q;
 printf(%s:\n, argv[i]);
 
-q = show_cgroup_by_path(argv[i], NULL, 0, arg_kernel_threads, arg_all);
+q = show_cgroup_by_path(argv[i], NULL, 0, arg_kernel_threads, arg_all, 0);
 if (q  0)
 r = q;
 }
@@ -148,7 +148,7 @@ int main(int argc, char *argv[]) {
 
 if (path_startswith(p, /sys/fs/cgroup)) {
 printf(Working Directory %s:\n, p);
-r = show_cgroup_by_path(p, NULL, 0, arg_kernel_threads, arg_all);
+r = show_cgroup_by_path(p, NULL, 0, arg_kernel_threads, arg_all, 0);
 } else {
 char *root = NULL;
 const char *t = NULL;
@@ -163,7 +163,7 @@ int main(int argc, char *argv[]) {
 t = root[0] ? root : /;
 }
 
-r = show_cgroup(SYSTEMD_CGROUP_CONTROLLER, t, NULL, 0, arg_kernel_threads, arg_all);
+r = show_cgroup(SYSTEMD_CGROUP_CONTROLLER, t, NULL, 0, arg_kernel_threads, arg_all, 0);
 free(root);
 }
 
diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
index 8513634..c4a0901 100644
--- a/src/core/selinux-access.c
+++ b/src/core/selinux-access.c
@@ -236,7 +236,7 @@ static int bus_get_audit_data(
 if (r  0)
 return r;
 
-r = get_process_cmdline(pid, LINE_MAX, true, audit-cmdline);
+r = get_process_cmdline(pid, LINE_MAX, true, audit-cmdline, 0);
 if (r  0)
 return r;
 
@@ -372,7 +372,7 @@ static int get_audit_data(
 if (r  0)
 return r;
 
-r = get_process_cmdline(ucred.pid, LINE_MAX, true, audit-cmdline);
+r = get_process_cmdline(ucred.pid, LINE_MAX, true, audit-cmdline, 0);
 if (r  0)
 return r;
 
diff --git a/src/journal/coredump.c b/src/journal/coredump.c
index a507fc6..8dfadbd 100644
--- a/src/journal/coredump.c
+++ b/src/journal/coredump.c
@@ -205,7 +205,7 @@ int main(int argc, char* argv[]) {
 IOVEC_SET_STRING(iovec[j++], core_exe);
 }
 
-if (get_process_cmdline(pid, LINE_MAX, false, t) = 0) {
+if (get_process_cmdline(pid, LINE_MAX, false, t, 0) = 0) {
 core_cmdline =

Re: [systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2012-09-25 Thread Lennart Poettering 
On Tue, 25.09.12 15:36, Lukáš Nykrýn (lnyk...@redhat.com) wrote:

Heya,

 diff --git a/src/shared/logs-show.h b/src/shared/logs-show.h
 index 3e6b6e0..3b63a5d 100644
 --- a/src/shared/logs-show.h
 +++ b/src/shared/logs-show.h
 @@ -27,26 +27,6 @@
  
  #include util.h
  
 -typedef enum OutputMode {
 -OUTPUT_SHORT,
 -OUTPUT_SHORT_MONOTONIC,
 -OUTPUT_VERBOSE,
 -OUTPUT_EXPORT,
 -OUTPUT_JSON,
 -OUTPUT_JSON_PRETTY,
 -OUTPUT_CAT,
 -_OUTPUT_MODE_MAX,
 -_OUTPUT_MODE_INVALID = -1
 -} OutputMode;
 -
 -typedef enum OutputFlags {
 -OUTPUT_SHOW_ALL   = 1  0,
 -OUTPUT_FOLLOW = 1  1,
 -OUTPUT_WARN_CUTOFF= 1  2,
 -OUTPUT_FULL_WIDTH = 1  3,
 -OUTPUT_COLOR  = 1  4
 -} OutputFlags;
 -

Hmm, I don't think this should be part of util.[ch] really, it's far to
specific to be considered just a utility. Could you please move this to a new
file output.h or so?

 -int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, 
 char **line) {
 -char *r, *k;
 +int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, 
 char **line, OutputFlags flags) {
 +char *r = NULL, *k;
  int c;
 -bool space = false;
 -size_t left;
  FILE *f;

I don't think this flag really should be passed here, this call is too
low-level for that. Instead, max_length == 0 could be used as a good
indicator for as big as needed or so.

Otherwise looks pretty OK!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] do not ellipsize cgroup members in full status

2012-09-20 Thread Lukáš Nykrýn 
Hello,
From bug report: https://bugzilla.redhat.com/show_bug.cgi?id=858693
There is no easy way how to force systemctl status not to crop long
lines with ellipsis in a virtual terminal. -a or --full options
doesn't help (and they even shouldn't help, according to the man page)

I am not sure if there should be additional parameter or extend --full,
but systemctl has already enough parameters.

Regards
Lukas
From a7f8b7492c273b3878122822f517be7d7e185b5c Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Thu, 20 Sep 2012 16:17:52 +0200
Subject: [PATCH] systemctl: do not ellipsize cgroup members in full status

---
 man/systemctl.xml |2 +-
 src/shared/cgroup-show.c  |   13 ---
 src/shared/util.c |   79 +++-
 src/systemctl/systemctl.c |   15 +---
 4 files changed, 65 insertions(+), 44 deletions(-)

diff --git a/man/systemctl.xml b/man/systemctl.xml
index fedc588..eacd2ed 100644
--- a/man/systemctl.xml
+++ b/man/systemctl.xml
@@ -151,7 +151,7 @@
 termoption--full/option/term
 
 listitemparaDo not ellipsize unit
-names and truncate unit descriptions
+names, cgroup members and truncate unit descriptions
 in the output of
 commandlist-units/command and
 commandlist-jobs/command./para/listitem
diff --git a/src/shared/cgroup-show.c b/src/shared/cgroup-show.c
index 9003a12..1b9f3cd 100644
--- a/src/shared/cgroup-show.c
+++ b/src/shared/cgroup-show.c
@@ -75,11 +75,12 @@ static void show_pid_array(int pids[], unsigned n_pids, const char *prefix, unsi
 /* And sort */
 qsort(pids, n_pids, sizeof(pid_t), compare);
 
-if (n_columns  8)
-n_columns -= 8;
-else
-n_columns = 20;
-
+if (n_columns != UINT_MAX) {
+if (n_columns  8)
+n_columns -= 8;
+else
+n_columns = 20;
+}
 for (i = 0; i  n_pids; i++) {
 char *t = NULL;
 
@@ -217,7 +218,7 @@ int show_cgroup_by_path(const char *path, const char *prefix, unsigned n_columns
 }
 }
 
-show_cgroup_by_path(last, p1, n_columns-2, kernel_threads, all);
+show_cgroup_by_path(last, p1, n_columns == UINT_MAX ? n_columns : n_columns-2, kernel_threads, all);
 free(last);
 }
 
diff --git a/src/shared/util.c b/src/shared/util.c
index 02ee637..570da7c 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -972,10 +972,8 @@ int get_process_comm(pid_t pid, char **name) {
 }
 
 int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char **line) {
-char *r, *k;
+char *r = NULL, *k;
 int c;
-bool space = false;
-size_t left;
 FILE *f;
 
 assert(max_length  0);
@@ -994,47 +992,66 @@ int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char *
 
 if (!f)
 return -errno;
+if (max_length != UINT_MAX) {
+bool space = false;
+size_t left;
+r = new(char, max_length);
+if (!r) {
+fclose(f);
+return -ENOMEM;
+}
 
-r = new(char, max_length);
-if (!r) {
-fclose(f);
-return -ENOMEM;
-}
+k = r;
+left = max_length;
+while ((c = getc(f)) != EOF) {
+
+if (isprint(c)) {
+if (space) {
+if (left = 4)
+break;
 
-k = r;
-left = max_length;
-while ((c = getc(f)) != EOF) {
+*(k++) = ' ';
+left--;
+space = false;
+}
 
-if (isprint(c)) {
-if (space) {
 if (left = 4)
 break;
 
-*(k++) = ' ';
+*(k++) = (char) c;
 left--;
-space = false;
-}
-
-if (left = 4)
-break;
+}  else
+space = true;
+}
 
-*(k++) = (char) c;
-left--;
-}  else
-space = true;
+