Doesn't really make sense, as it is always path to the python executable. bz#741255 --- src/include/problem_data.h | 3 +++ src/lib/problem_data.c | 5 +++++ src/report-python/problem_data.c | 8 ++++++++ 3 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/include/problem_data.h b/src/include/problem_data.h index 14d1240..87037a2 100644 --- a/src/include/problem_data.h +++ b/src/include/problem_data.h @@ -74,6 +74,9 @@ void add_to_problem_data(problem_data_t *problem_data, const char *name, const char *content);
+/* Removes item from problem data */ +void remove_from_problem_data(problem_data_t *problem_data, const char *key); + static inline struct problem_item *get_problem_data_item_or_NULL(problem_data_t *problem_data, const char *key) { return (struct problem_item *)g_hash_table_lookup(problem_data, key); diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c index 2ddff86..7d85709 100644 --- a/src/lib/problem_data.c +++ b/src/lib/problem_data.c @@ -155,6 +155,11 @@ void add_to_problem_data(problem_data_t *problem_data, add_to_problem_data_ext(problem_data, name, content, CD_FLAG_TXT + CD_FLAG_ISNOTEDITABLE); }
+void remove_from_problem_data(problem_data_t *problem_data, const char *key) +{ + g_hash_table_remove(problem_data, key); +} + const char *get_problem_item_content_or_die(problem_data_t *problem_data, const char *key) { struct problem_item *item = get_problem_data_item_or_NULL(problem_data, key); diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c index 9067dea..8b4088d 100644 --- a/src/report-python/problem_data.c +++ b/src/report-python/problem_data.c @@ -115,8 +115,16 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject *pself, PyObject * static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null) { p_problem_data *self = (p_problem_data*)pself; + int have_executable = (get_problem_data_item_or_NULL(self->cd, "executable") != NULL); + add_basics_to_problem_data(self->cd);
+ /* Workaround - the executable item will always be /usr/bin/python, which is useless. */ + if (!have_executable) + { + remove_from_problem_data(self->cd, "executable"); + } + Py_RETURN_NONE; }
On Thu, 2011-10-20 at 12:31 +0200, Martin Milata wrote:
Doesn't really make sense, as it is always path to the python executable. bz#741255
I cannot agree with you. As I see the purpose of the "executable: SOMETHING" item in the bugreport it's used to differentiate between running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to the /usr/bin/vim). According to the value of argv[0] vim decides how it should run. And you can do exactly the same in Python and sys.argv[0].
And even if this item should have been removed this wouldn't have been the right way to do it -- let handlers query and pass this info and then remove it somewhere else?
If you look at the https://bugzilla.redhat.com/show_bug.cgi?id=747562 which is a testing bugreport for my python-meh and libreport patches "executable: /usr/sbin/firstboot" is redundant here. And I think the righ way how to report this info is to use "cmdline: SOMETHING" containing (in case of python script): "cmdline: PATH_TO_PYTHON PYTHON_OPTIONS EXECUTABLE EXECUTABLE_OPTIONS"
for example: "cmdline: /usr/bin/python -t -Q /usr/bin/phatch halved.phatch"
Which gives much more information (e.g. python 2 vs. python 3?) and includes also the "executable".
I don't think it would resolve bz#741255. But this should be solved somewhere else.
-- Vratislav Podzimek
src/include/problem_data.h | 3 +++ src/lib/problem_data.c | 5 +++++ src/report-python/problem_data.c | 8 ++++++++ 3 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/include/problem_data.h b/src/include/problem_data.h index 14d1240..87037a2 100644 --- a/src/include/problem_data.h +++ b/src/include/problem_data.h @@ -74,6 +74,9 @@ void add_to_problem_data(problem_data_t *problem_data, const char *name, const char *content);
+/* Removes item from problem data */ +void remove_from_problem_data(problem_data_t *problem_data, const char *key);
static inline struct problem_item *get_problem_data_item_or_NULL(problem_data_t *problem_data, const char *key) { return (struct problem_item *)g_hash_table_lookup(problem_data, key); diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c index 2ddff86..7d85709 100644 --- a/src/lib/problem_data.c +++ b/src/lib/problem_data.c @@ -155,6 +155,11 @@ void add_to_problem_data(problem_data_t *problem_data, add_to_problem_data_ext(problem_data, name, content, CD_FLAG_TXT + CD_FLAG_ISNOTEDITABLE); }
+void remove_from_problem_data(problem_data_t *problem_data, const char *key) +{
- g_hash_table_remove(problem_data, key);
+}
const char *get_problem_item_content_or_die(problem_data_t *problem_data, const char *key) { struct problem_item *item = get_problem_data_item_or_NULL(problem_data, key); diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c index 9067dea..8b4088d 100644 --- a/src/report-python/problem_data.c +++ b/src/report-python/problem_data.c @@ -115,8 +115,16 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject *pself, PyObject * static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null) { p_problem_data *self = (p_problem_data*)pself;
int have_executable = (get_problem_data_item_or_NULL(self->cd, "executable") != NULL);
add_basics_to_problem_data(self->cd);
/* Workaround - the executable item will always be /usr/bin/python, which is useless. */
if (!have_executable)
{
remove_from_problem_data(self->cd, "executable");
}
Py_RETURN_NONE;
}
On 21.10.2011 12:16, Vratislav Podzimek wrote:
I cannot agree with you. As I see the purpose of the "executable: SOMETHING" item in the bugreport it's used to differentiate between running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to the /usr/bin/vim). According to the value of argv[0] vim decides how it should run. And you can do exactly the same in Python and sys.argv[0].
Two different use-cases (Python exception vs. SELinux denial). Sealert always sets executable to python which is not helpful at all.
On 20.10.2011 12:31, Martin Milata wrote:
Doesn't really make sense, as it is always path to the python executable. bz#741255
src/include/problem_data.h | 3 +++ src/lib/problem_data.c | 5 +++++ src/report-python/problem_data.c | 8 ++++++++ 3 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/include/problem_data.h b/src/include/problem_data.h index 14d1240..87037a2 100644 --- a/src/include/problem_data.h +++ b/src/include/problem_data.h @@ -74,6 +74,9 @@ void add_to_problem_data(problem_data_t *problem_data, const char *name, const char *content);
+/* Removes item from problem data */ +void remove_from_problem_data(problem_data_t *problem_data, const char *key);
static inline struct problem_item *get_problem_data_item_or_NULL(problem_data_t *problem_data, const char *key) { return (struct problem_item *)g_hash_table_lookup(problem_data, key); diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c index 2ddff86..7d85709 100644 --- a/src/lib/problem_data.c +++ b/src/lib/problem_data.c @@ -155,6 +155,11 @@ void add_to_problem_data(problem_data_t *problem_data, add_to_problem_data_ext(problem_data, name, content, CD_FLAG_TXT + CD_FLAG_ISNOTEDITABLE); }
+void remove_from_problem_data(problem_data_t *problem_data, const char *key) +{
- g_hash_table_remove(problem_data, key);
+}
const char *get_problem_item_content_or_die(problem_data_t *problem_data, const char *key) { struct problem_item *item = get_problem_data_item_or_NULL(problem_data, key); diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c index 9067dea..8b4088d 100644 --- a/src/report-python/problem_data.c +++ b/src/report-python/problem_data.c @@ -115,8 +115,16 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject *pself, PyObject * static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null) { p_problem_data *self = (p_problem_data*)pself;
int have_executable = (get_problem_data_item_or_NULL(self->cd, "executable") != NULL);
add_basics_to_problem_data(self->cd);
/* Workaround - the executable item will always be /usr/bin/python, which is useless. */
if (!have_executable)
{
remove_from_problem_data(self->cd, "executable");
}
Py_RETURN_NONE;
}
This *always* removes the executable field, when called from Python binding. That will (at least) result into a different behavior of C and Python API. If I understand correctly, what we want to do is to remove the executable (or executable == /usr/bin/python ?) from *SELinux* report, no matter if called from C or Python.
M.
On Fri, Oct 21, 2011 at 12:50:17 +0200, Michal Toman wrote:
On 21.10.2011 12:16, Vratislav Podzimek wrote:
I cannot agree with you. As I see the purpose of the "executable: SOMETHING" item in the bugreport it's used to differentiate between running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to the /usr/bin/vim). According to the value of argv[0] vim decides how it should run. And you can do exactly the same in Python and sys.argv[0].
Two different use-cases (Python exception vs. SELinux denial). Sealert always sets executable to python which is not helpful at all.
I see -- I overlooked the other use-case and assumed that the python bindings are only used to report problems which have nothing to do with current process (as is the case with Sealert).
On 20.10.2011 12:31, Martin Milata wrote:
Doesn't really make sense, as it is always path to the python executable. bz#741255
[patch]
This *always* removes the executable field, when called from Python binding. That will (at least) result into a different behavior of C and Python API. If I understand correctly, what we want to do is to remove the executable (or executable == /usr/bin/python ?) from *SELinux* report, no matter if called from C or Python.
I tried to solve it in a different way, patch should arrive as a reply to this message.
Generally, I think that what we want to do is provide interfaces for creating two kinds of reports -- reports about current process, where we can detect things like executable, and reports about something else, which is the case with sealert and where the caller should provide all the information.
Do not call add_basics() in createAlertSignature, which is probably only used by Sealert. The method detects and adds executable item containing path to the executable of the current process, which is useless as the report is not about current process. --- src/report-python/__init__.py | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/report-python/__init__.py b/src/report-python/__init__.py index 7955878..ac1da3a 100644 --- a/src/report-python/__init__.py +++ b/src/report-python/__init__.py @@ -137,7 +137,7 @@ def createAlertSignature(component, hashmarkername, hashvalue, summary, alertSig pd.add("duphash", hashvalue) pd.add("reason", summary) pd.add("description", alertSignature) - pd.add_basics() + pd.add("analyzer", "libreport")
return pd
On Wed, 2011-10-26 at 12:47 +0200, Martin Milata wrote:
On Fri, Oct 21, 2011 at 12:50:17 +0200, Michal Toman wrote:
On 21.10.2011 12:16, Vratislav Podzimek wrote:
I cannot agree with you. As I see the purpose of the "executable: SOMETHING" item in the bugreport it's used to differentiate between running /usr/bin/vim and /usr/bin/vimdiff (which is a symlink to the /usr/bin/vim). According to the value of argv[0] vim decides how it should run. And you can do exactly the same in Python and sys.argv[0].
Two different use-cases (Python exception vs. SELinux denial). Sealert always sets executable to python which is not helpful at all.
I see -- I overlooked the other use-case and assumed that the python bindings are only used to report problems which have nothing to do with current process (as is the case with Sealert).
On 20.10.2011 12:31, Martin Milata wrote:
Doesn't really make sense, as it is always path to the python executable. bz#741255
[patch]
This *always* removes the executable field, when called from Python binding. That will (at least) result into a different behavior of C and Python API. If I understand correctly, what we want to do is to remove the executable (or executable == /usr/bin/python ?) from *SELinux* report, no matter if called from C or Python.
I tried to solve it in a different way, patch should arrive as a reply to this message.
Generally, I think that what we want to do is provide interfaces for creating two kinds of reports -- reports about current process, where we can detect things like executable, and reports about something else, which is the case with sealert and where the caller should provide all the information.
I think that the key is not to override info provided from "caller" (like in the attached patch) and patch "callers" to provide it correctly. It's the same for python-meh where I'am doing it right now.
-- Vratislav Podzimek
Here's another attempt at solving the bug. It adds a flag to add_basics_to_problem_data indicating whether or not we are interested about data from current process. Additionally, it adds optional argument to createAlertSignature, which can be used by sealert to pass the executable directly.
Suggestions are welcome.
Martin
-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8-->8--
- add_basics_to_problem_data takes parameter indicating whether we are adding information about current process - function for adding basic data not from the current process is added to the python api - createAlertSignature takes optional argument that can be used to manually set the executable --- src/include/problem_data.h | 4 +++- src/lib/problem_data.c | 4 ++-- src/report-python/__init__.py | 6 ++++-- src/report-python/problem_data.c | 19 ++++++++++++++----- 4 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/include/problem_data.h b/src/include/problem_data.h index 14d1240..f332433 100644 --- a/src/include/problem_data.h +++ b/src/include/problem_data.h @@ -57,7 +57,9 @@ typedef GHashTable problem_data_t;
problem_data_t *new_problem_data(void);
-void add_basics_to_problem_data(problem_data_t *pd); +/* If this_process is true, it means we are adding information about current + * process and can e.g. read path of the executable from /proc. */ +void add_basics_to_problem_data(problem_data_t *pd, int this_process);
static inline void free_problem_data(problem_data_t *problem_data) { diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c index 9d45d3e..7f81ea3 100644 --- a/src/lib/problem_data.c +++ b/src/lib/problem_data.c @@ -57,7 +57,7 @@ problem_data_t *new_problem_data(void) free, free_problem_item); }
-void add_basics_to_problem_data(problem_data_t *pd) +void add_basics_to_problem_data(problem_data_t *pd, int this_process) { const char *analyzer = get_problem_item_content_or_NULL(pd, FILENAME_ANALYZER); if (analyzer == NULL) @@ -106,7 +106,7 @@ void add_basics_to_problem_data(problem_data_t *pd)
pid_t pid = getpid(); const char *executable = get_problem_item_content_or_NULL(pd, FILENAME_EXECUTABLE); - if (executable == NULL && pid > 0) + if (executable == NULL && this_process && pid > 0) { char buf[PATH_MAX+1]; char *exe = xasprintf("/proc/%u/exe", pid); diff --git a/src/report-python/__init__.py b/src/report-python/__init__.py index 6f4f543..a7f7dc3 100644 --- a/src/report-python/__init__.py +++ b/src/report-python/__init__.py @@ -130,14 +130,16 @@ def getVersion():
return _hardcoded_default_version
-def createAlertSignature(component, hashmarkername, hashvalue, summary, alertSignature): +def createAlertSignature(component, hashmarkername, hashvalue, summary, alertSignature, executable=None): pd = problem_data() pd.add("component", component) pd.add("hashmarkername", hashmarkername) pd.add("duphash", hashvalue) pd.add("reason", summary) pd.add("description", alertSignature) - pd.add_basics() + if executable: + pd.add("executable", executable) + pd.add_basics_other_process()
return pd
diff --git a/src/report-python/problem_data.c b/src/report-python/problem_data.c index 9067dea..791c439 100644 --- a/src/report-python/problem_data.c +++ b/src/report-python/problem_data.c @@ -115,7 +115,15 @@ static PyObject *p_create_dump_dir_from_problem_data(PyObject *pself, PyObject * static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_null) { p_problem_data *self = (p_problem_data*)pself; - add_basics_to_problem_data(self->cd); + add_basics_to_problem_data(self->cd, 1); + + Py_RETURN_NONE; +} + +static PyObject *p_add_basics_to_problem_data_other_proc(PyObject *pself, PyObject *always_null) +{ + p_problem_data *self = (p_problem_data*)pself; + add_basics_to_problem_data(self->cd, 0);
Py_RETURN_NONE; } @@ -127,10 +135,11 @@ static PyObject *p_add_basics_to_problem_data(PyObject *pself, PyObject *always_
static PyMethodDef p_problem_data_methods[] = { /* method_name, func, flags, doc_string */ - { "add" , p_problem_data_add , METH_VARARGS }, - { "get" , p_get_problem_data_item , METH_VARARGS }, - { "create_dump_dir", p_create_dump_dir_from_problem_data, METH_VARARGS }, - { "add_basics", p_add_basics_to_problem_data, METH_NOARGS }, + { "add" , p_problem_data_add , METH_VARARGS }, + { "get" , p_get_problem_data_item , METH_VARARGS }, + { "create_dump_dir" , p_create_dump_dir_from_problem_data , METH_VARARGS }, + { "add_basics" , p_add_basics_to_problem_data , METH_NOARGS }, + { "add_basics_other_process", p_add_basics_to_problem_data_other_proc, METH_NOARGS }, { NULL } };
Here's another attempt at solving the bug. It adds a flag to add_basics_to_problem_data indicating whether or not we are interested about data from current process. Additionally, it adds optional argument to createAlertSignature, which can be used by sealert to pass the executable directly.
ok, patch seems good. what I would like to see is only addition of an optional argument to *createAlertSignature* and let setroubleshoot and anaconda fix this issue.
On 03/19/2012 11:58 AM, Nikola Pajkovsky wrote:
Here's another attempt at solving the bug. It adds a flag to add_basics_to_problem_data indicating whether or not we are interested about data from current process. Additionally, it adds optional argument to createAlertSignature, which can be used by sealert to pass the executable directly.
ok, patch seems good. what I would like to see is only addition of an optional argument to *createAlertSignature* and let setroubleshoot and anaconda fix this issue.
- Vrata is working on a patch for python-meh and abrt-server to do exactly this..
Vrato, what's the news?
Thanks, Jirka
On Mon, 2012-03-19 at 12:49 +0100, Jiri Moskovcak wrote:
On 03/19/2012 11:58 AM, Nikola Pajkovsky wrote:
Here's another attempt at solving the bug. It adds a flag to add_basics_to_problem_data indicating whether or not we are interested about data from current process. Additionally, it adds optional argument to createAlertSignature, which can be used by sealert to pass the executable directly.
ok, patch seems good. what I would like to see is only addition of an optional argument to *createAlertSignature* and let setroubleshoot and anaconda fix this issue.
- Vrata is working on a patch for python-meh and abrt-server to do
exactly this..
Am I? I am working on modifying abrt-server in a way that it will be possible for python-meh to use the abrt.socket for communication.
Vrato, what's the news?
I believe it's almost done, but since I have some hurrying Anaconda stuff now, these things are going slowly. I will send my patches for a review this week. Once they are approved I'll do the modifications in python-meh.
Thanks, Jirka
crash-catcher@lists.fedorahosted.org