[PATCH 0/4] vmcore patches

Signed-off-by: Richard Marko <rmarko-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
src/hooks/abrt_harvest_vmcore.py.in | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/hooks/abrt_harvest_vmcore.py.in b/src/hooks/abrt_harvest_vmcore.py.in
index 4f2e1d0..f2ac4eb 100644
--- a/src/hooks/abrt_harvest_vmcore.py.in
+++ b/src/hooks/abrt_harvest_vmcore.py.in
@@ -47,10 +47,17 @@ def parse_kdump():
This function parses /etc/kdump.conf to get a path to kdump's
dump directory.
"""
+ # default
+ dump_path = '/var/crash'

# filesystem types that can be used by kdump for dumping
fs_types = ['ext4', 'ext3', 'ext2', 'minix', 'btrfs', 'xfs']

+ if not os.access('/etc/kdump.conf', os.R_OK):
+ sys.stderr.write("/etc/kdump.conf not readable, using "
+ "default path '%s'\n" % dump_path)
+ return dump_path
+
with open('/etc/kdump.conf') as conf_file:
kdump_conf = conf_file.read()

@@ -59,9 +66,6 @@ def parse_kdump():
result = regexp.search(kdump_conf)
if result:
dump_path = result.group(1).strip()
- else:
- # default
- dump_path = '/var/crash'

# default
partition = None

--
1.8.3.1

Jiri Moskovcak

2013-09-02 11:01:20 UTC

Post by Richard Marko
---
src/hooks/abrt_harvest_vmcore.py.in | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/hooks/abrt_harvest_vmcore.py.in b/src/hooks/abrt_harvest_vmcore.py.in
index 4f2e1d0..f2ac4eb 100644
--- a/src/hooks/abrt_harvest_vmcore.py.in
+++ b/src/hooks/abrt_harvest_vmcore.py.in
This function parses /etc/kdump.conf to get a path to kdump's
dump directory.
"""
+ # default
+ dump_path = '/var/crash'
# filesystem types that can be used by kdump for dumping
fs_types = ['ext4', 'ext3', 'ext2', 'minix', 'btrfs', 'xfs']
+ sys.stderr.write("/etc/kdump.conf not readable, using "
+ "default path '%s'\n" % dump_path)
+ return dump_path
+

- please factor out the logging functions from abrt-action-analyze-core
script and use it in both scripts
- they also exists in the reportclient module in libreport, so maybe you
can use them instead of creating just another abrt module

Post by Richard Marko
kdump_conf = conf_file.read()
result = regexp.search(kdump_conf)
dump_path = result.group(1).strip()
- # default
- dump_path = '/var/crash'
# default
partition = None

Richard Marko

2013-09-02 11:21:08 UTC

Post by Richard Marko
+ sys.stderr.write("/etc/kdump.conf not readable, using "
+ "default path '%s'\n" % dump_path)
+ return dump_path
+

- please factor out the logging functions from
abrt-action-analyze-core script and use it in both scripts
- they also exists in the reportclient module in libreport, so maybe
you can use them instead of creating just another abrt module

I've had a version which used syslog module instead of writing to stderr
but I've replaced it as the rest of the code uses stderr and I'll
be fixing this everywhere due to rhbz#1001139.

The question is where should these scripts log by default - to stderr or
to syslog?

I think using syslog is better than current situation.

--
Richard Marko

Jiri Moskovcak

2013-09-03 06:26:00 UTC

Post by Richard Marko
+ sys.stderr.write("/etc/kdump.conf not readable, using "
+ "default path '%s'\n" % dump_path)
+ return dump_path
+

- please factor out the logging functions from
abrt-action-analyze-core script and use it in both scripts
- they also exists in the reportclient module in libreport, so maybe
you can use them instead of creating just another abrt module

I've had a version which used syslog module instead of writing to stderr
but I've replaced it as the rest of the code uses stderr and I'll
be fixing this everywhere due to rhbz#1001139.
The question is where should these scripts log by default - to stderr or
to syslog?
I think using syslog is better than current situation.

- It depends on availability of tty, if tty is available (means the app
were started from the terminal) I would log to stderr, otherwise to syslog
- but please make sure that only really important problems are written
to syslog if the verbosity == 0

--Jirka

Richard Marko

2013-09-03 14:30:01 UTC

Post by Richard Marko
+ sys.stderr.write("/etc/kdump.conf not readable, using "
+ "default path '%s'\n" % dump_path)
+ return dump_path
+

- please factor out the logging functions from
abrt-action-analyze-core script and use it in both scripts
- they also exists in the reportclient module in libreport, so maybe
you can use them instead of creating just another abrt module

I've had a version which used syslog module instead of writing to stderr
but I've replaced it as the rest of the code uses stderr and I'll
be fixing this everywhere due to rhbz#1001139.
The question is where should these scripts log by default - to stderr or
to syslog?
I think using syslog is better than current situation.

- It depends on availability of tty, if tty is available (means the
app were started from the terminal) I would log to stderr, otherwise
to syslog
- but please make sure that only really important problems are written
to syslog if the verbosity == 0

Should be possible to implement with isatty. I was thinking about
following: logging functions factored out would
- log to stderr if stderr is real tty
- log to syslog if running as daemon
- [later] log to journal via it's native api

Regarding verbosity, I would solve this with proper log levels
(log_debug, log_warn, log_error) which would pass level to
syslog/journal where filtering is available while using verbosity when
stderr is used.

--
Richard Marko

Richard Marko

2013-09-02 09:27:01 UTC

Signed-off-by: Richard Marko <rmarko-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
abrt.spec.in | 1 +
1 file changed, 1 insertion(+)

diff --git a/abrt.spec.in b/abrt.spec.in
index ef20425..007d4d1 100644
--- a/abrt.spec.in
+++ b/abrt.spec.in
@@ -201,6 +201,7 @@ Group: System Environment/Libraries
Requires: %{name} = %{version}-%{release}
Requires: abrt-addon-kerneloops
Requires: crash
+Requires: kexec-tools

%description addon-vmcore
This package contains plugin for collecting kernel crash information from

--
1.8.3.1

Richard Marko

2013-09-02 09:27:02 UTC

Previously os.listdir on inaccessible dump_dir resulted
in exception. This replaces insufficient os.path.exists check
(which is already handled in abrt-vmcore.service file) with
os.access call which checks for dir readability.

Signed-off-by: Richard Marko <rmarko-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
src/hooks/abrt_harvest_vmcore.py.in | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/hooks/abrt_harvest_vmcore.py.in b/src/hooks/abrt_harvest_vmcore.py.in
index 3951307..ea331c0 100644
--- a/src/hooks/abrt_harvest_vmcore.py.in
+++ b/src/hooks/abrt_harvest_vmcore.py.in
@@ -186,8 +186,10 @@ def harvest_vmcore():

dump_dir = parse_kdump()

- if not os.path.exists(dump_dir):
- sys.exit(0)
+ if not os.access(dump_dir, os.R_OK):
+ sys.stderr.write("Dump directory '%s' not accessible. "
+ "Exiting.\n" % dump_dir)
+ sys.exit(1)

# Wait for abrtd to start. Give it at least 1 second to initialize.
for i in xrange(10):

--
1.8.3.1

Jiri Moskovcak

2013-09-03 06:41:25 UTC

Post by Richard Marko
Previously os.listdir on inaccessible dump_dir resulted
in exception. This replaces insufficient os.path.exists check
(which is already handled in abrt-vmcore.service file) with
os.access call which checks for dir readability.
---
src/hooks/abrt_harvest_vmcore.py.in | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/hooks/abrt_harvest_vmcore.py.in b/src/hooks/abrt_harvest_vmcore.py.in
index 3951307..ea331c0 100644
--- a/src/hooks/abrt_harvest_vmcore.py.in
+++ b/src/hooks/abrt_harvest_vmcore.py.in
dump_dir = parse_kdump()
- sys.exit(0)
+ sys.stderr.write("Dump directory '%s' not accessible. "
+ "Exiting.\n" % dump_dir)
+ sys.exit(1)

- even with this improved code it can throw an exception, because it's
racy, so the exception should be handled also later when the code is
accessing the dump_dir (line 219+)

Post by Richard Marko
# Wait for abrtd to start. Give it at least 1 second to initialize.

Richard Marko

2013-09-02 09:27:00 UTC

Signed-off-by: Richard Marko <rmarko-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
src/hooks/abrt_harvest_vmcore.py.in | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/hooks/abrt_harvest_vmcore.py.in b/src/hooks/abrt_harvest_vmcore.py.in
index f2ac4eb..3951307 100644
--- a/src/hooks/abrt_harvest_vmcore.py.in
+++ b/src/hooks/abrt_harvest_vmcore.py.in
@@ -62,7 +62,7 @@ def parse_kdump():
kdump_conf = conf_file.read()

# first uncommented path instruction
- regexp = re.compile('^(?<!#)path (.*)$', flags=8) # MULTILINE
+ regexp = re.compile('^(?<!#)path (.*)$', flags=re.MULTILINE)
result = regexp.search(kdump_conf)
if result:
dump_path = result.group(1).strip()
@@ -71,7 +71,7 @@ def parse_kdump():
partition = None
# first uncommented fs_type partition instruction
for fs_type in fs_types:
- regexp = re.compile('^(?<!#)' + fs_type + ' (.*)$', flags=8)
+ regexp = re.compile('^(?<!#)' + fs_type + ' (.*)$', flags=re.MULTILINE)
result = regexp.search(kdump_conf)
if result:
partition = result.group(1)

--
1.8.3.1

Jiri Moskovcak

2013-09-02 10:58:16 UTC

- missing ticket number

Post by Richard Marko
Mostly fixing error handling + adding dependency on kexec-tools.
Please note that it still produces
selinux AVC when trying to access /var/crash on fedora 19.
(I'll take care of the AVC)

- is there a ticket for it? (there should be a bz ticket if selinux
policy change is required)

Post by Richard Marko
vmcore: don't fail if /etc/kdump.conf is not readable
vmcore: use re.MULTILINE instead of numerical value
spec: vmcore: require kexec-tools
vmcore: fail gracefully if dump_dir is not accessible
abrt.spec.in | 1 +
src/hooks/abrt_harvest_vmcore.py.in | 20 +++++++++++++-------
2 files changed, 14 insertions(+), 7 deletions(-)

Richard Marko

2013-09-02 11:14:12 UTC

Post by Jiri Moskovcak
- missing ticket number

#687, will add it if there will be v2 patches, please add it otherwise.

- is there a ticket for it? (there should be a bz ticket if selinux
policy change is required)

No and there's no change in behavior either - this should have worked
even before my patches.

--
Richard Marko

Jiri Moskovcak

2013-09-03 06:37:32 UTC

Post by Jiri Moskovcak
- missing ticket number

#687, will add it if there will be v2 patches, please add it otherwise.

- is there a ticket for it? (there should be a bz ticket if selinux
policy change is required)

No and there's no change in behavior either - this should have worked
even before my patches.

- I;m not saying it's connected to your patches, but the problem exists,
so there should be a ticket either against abrt (if it's our problem) or
against selinux-policy (if it's their problem)

Richard Marko

2013-09-03 12:16:04 UTC

Post by Jiri Moskovcak
- missing ticket number

#687, will add it if there will be v2 patches, please add it otherwise.