make BZ insert small text attachments inline; move text file detection code

Run-tested

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

8 files changed, 154 insertions, 162 deletions

diff --git a/inc/CrashTypes.h b/inc/CrashTypes.h
index 5149fb53..77947f6f 100644
--- a/inc/CrashTypes.h
+++ b/inc/CrashTypes.h
@@ -7,6 +7,8 @@
 // BIN - binary value, should be displayed as a path to binary file
 // TXT - text value, should be displayed
 // ATT - text value which can be sent as attachment via reporters
+// TODO: maybe we don't need separate CD_ATT - can simply look at strlen(content)
+// in all places which want to handle "long" and "short" texts differently
 #define CD_SYS          "s"
 #define CD_BIN          "b"
 #define CD_TXT          "t"
diff --git a/lib/Plugins/Bugzilla.cpp b/lib/Plugins/Bugzilla.cpp
index 7ddacf7c..57342389 100644
--- a/lib/Plugins/Bugzilla.cpp
+++ b/lib/Plugins/Bugzilla.cpp
@@ -14,13 +14,6 @@
 #define XML_RPC_SUFFIX "/xmlrpc.cgi"
 
 
-static void create_new_bug_description(const map_crash_report_t& pCrashReport, std::string& pDescription)
-{
-    pDescription = "abrt "VERSION" detected a crash.\n\n";
-    pDescription += make_description_bz(pCrashReport);
-}
-
-
 /*
  * Static namespace for xmlrpc stuff.
  * Used mainly to ensure we always destroy xmlrpc client and server_info.
@@ -236,8 +229,8 @@ uint32_t ctx::new_bug(const map_crash_report_t& pCrashReport)
     std::string summary = "[abrt] crash detected in " + package;
     std::string status_whiteboard = "abrt_hash:" + uuid;
 
-    std::string description;
-    create_new_bug_description(pCrashReport, description);
+    std::string description = "abrt "VERSION" detected a crash.\n\n";
+    description += make_description_bz(pCrashReport);
 
     std::string product;
     std::string version;
@@ -287,15 +280,17 @@ void ctx::add_attachments(const char* bug_id_str, const map_crash_report_t& pCra
     map_crash_report_t::const_iterator it = pCrashReport.begin();
     for (; it != pCrashReport.end(); it++)
     {
-        if (it->second[CD_TYPE] == CD_ATT)
+        const std::string &filename = it->first;
+        const std::string &type = it->second[CD_TYPE];
+        const std::string &content = it->second[CD_CONTENT];
+
+        if (type == CD_ATT)
         {
-            std::string description = "File: " + it->first;
-            const std::string& to_encode = it->second[CD_CONTENT];
-            char *encoded64 = encode_base64(to_encode.c_str(), to_encode.length());
+            char *encoded64 = encode_base64(content.c_str(), content.length());
             xmlrpc_value* param = xmlrpc_build_value(&env,"(s{s:s,s:s,s:s,s:s})",
                                               bug_id_str,
-                                              "description", description.c_str(),
-                                              "filename", it->first.c_str(),
+                                              "description", ("File: " + filename).c_str(),
+                                              "filename", filename.c_str(),
                                               "contenttype", "text/plain",
                                               "data", encoded64
                                       );
diff --git a/lib/Plugins/Mailx.cpp b/lib/Plugins/Mailx.cpp
index 32eeb25c..dc6e2b0e 100644
--- a/lib/Plugins/Mailx.cpp
+++ b/lib/Plugins/Mailx.cpp
@@ -90,6 +90,7 @@ std::string CMailx::Report(const map_crash_report_t& pCrashReport,
     unsigned arg_size = 0;
     args = append_str_to_vector(args, arg_size, MAILX_COMMAND);
 
+//TODO: move email body generation to make_descr.cpp
     std::string binaryFiles, commonFiles, bigTextFiles, additionalFiles, UUIDFile;
     map_crash_report_t::const_iterator it;
     for (it = pCrashReport.begin(); it != pCrashReport.end(); it++)
diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp
index 2883d01f..fff46957 100644
--- a/lib/Utils/DebugDump.cpp
+++ b/lib/Utils/DebugDump.cpp
@@ -23,7 +23,6 @@
 #include <iostream>
 #include <sstream>
 #include <sys/utsname.h>
-//#include <magic.h>
 #include "abrtlib.h"
 #include "DebugDump.h"
 #include "ABRTException.h"
@@ -287,72 +286,6 @@ static void DeleteFileDir(const char *pDir)
     }
 }
 
-static bool IsTextFile(const char *name)
-{
-    /* Some files in our dump directories are known to always be textual */
-    if (strcmp(name, "backtrace") == 0
-     || strcmp(name, "cmdline") == 0
-    ) {
-        return true;
-    }
-
-/* This idiotic library thinks that file containing just "0" is not text (!!)
-
-    magic_t m = magic_open(MAGIC_MIME_TYPE);
-
-    if (m == NULL)
-    {
-        throw CABRTException(EXCEP_ERROR, std::string(__func__) + "Cannot open magic cookie: " + magic_error(m));
-    }
-
-    int r = magic_load(m, NULL);
-
-    if (r == -1)
-    {
-        magic_close(m);
-        throw CABRTException(EXCEP_ERROR, std::string(__func__) + "Cannot load magic db: " + magic_error(m));
-    }
-
-    char* ch = (char *) magic_file(m, pName.c_str());
-
-    if (ch == NULL)
-    {
-        magic_close(m);
-        throw CABRTException(EXCEP_ERROR, std::string(__func__) + "Cannot determine file type: " + magic_error(m));
-    }
-
-    bool isText = (strncmp(ch, "text", 4) == 0);
-
-    magic_close(m);
-
-    return isText;
- */
-    int fd = open(name, O_RDONLY);
-    if (fd < 0)
-        return false;
-
-    unsigned char buf[4*1024];
-    int r = full_read(fd, buf, sizeof(buf));
-    close(fd);
-
-    /* Every once in a while, even a text file contains a few garbled
-     * or unexpected non-ASCII chars. We should not declare it "binary".
-     */
-    const unsigned RATIO = 50;
-    unsigned total_chars = r + RATIO;
-    unsigned bad_chars = 1; /* 1 prevents division by 0 later */
-    while (--r >= 0)
-    {
-        if (buf[r] >= 0x7f
-         /* among control chars, only '\t','\n' etc are allowed */
-         || (buf[r] < ' ' && !isspace(buf[r]))
-        ) {
-            bad_chars++;
-        }
-    }
-    return (total_chars / bad_chars) >= RATIO;
-}
-
 void CDebugDump::Delete()
 {
     if (!ExistFileDir(m_sDebugDumpDir.c_str()))
@@ -460,7 +393,7 @@ void CDebugDump::InitGetNextFile()
 {
     if (!m_bOpened)
     {
-        throw CABRTException(EXCEP_DD_OPEN, "CDebugDump::InitGetNextFile(): DebugDump is not opened.");
+        throw CABRTException(EXCEP_DD_OPEN, "DebugDump is not opened");
     }
     if (m_pGetNextFileDir != NULL)
     {
@@ -469,11 +402,11 @@ void CDebugDump::InitGetNextFile()
     m_pGetNextFileDir = opendir(m_sDebugDumpDir.c_str());
     if (m_pGetNextFileDir == NULL)
     {
-        throw CABRTException(EXCEP_DD_OPEN, "CDebugDump::InitGetNextFile(): Cannot open dir " + m_sDebugDumpDir);
+        throw CABRTException(EXCEP_DD_OPEN, "Can't open dir " + m_sDebugDumpDir);
     }
 }
 
-bool CDebugDump::GetNextFile(std::string& pFileName, std::string& pContent, bool& pIsTextFile)
+bool CDebugDump::GetNextFile(std::string *short_name, std::string *full_name)
 {
     if (m_pGetNextFileDir == NULL)
     {
@@ -485,19 +418,10 @@ bool CDebugDump::GetNextFile(std::string& pFileName, std::string& pContent, bool
     {
         if (is_regular_file(dent, m_sDebugDumpDir.c_str()))
         {
-            std::string fullname = concat_path_file(m_sDebugDumpDir.c_str(), dent->d_name);
-
-            pFileName = dent->d_name;
-            if (IsTextFile(fullname.c_str()))
-            {
-                LoadText(dent->d_name, pContent);
-                pIsTextFile = true;
-            }
-            else
-            {
-                pContent.clear();
-                pIsTextFile = false;
-            }
+            if (short_name)
+                *short_name = dent->d_name;
+            if (full_name)
+                *full_name = concat_path_file(m_sDebugDumpDir.c_str(), dent->d_name);
             return true;
         }
     }
diff --git a/lib/Utils/DebugDump.h b/lib/Utils/DebugDump.h
index b48a386d..d7533534 100644
--- a/lib/Utils/DebugDump.h
+++ b/lib/Utils/DebugDump.h
@@ -72,7 +72,8 @@ class CDebugDump
         void SaveBinary(const char* pName, const char* pData, unsigned pSize);
 
         void InitGetNextFile();
-        bool GetNextFile(std::string& pFileName, std::string& pContent, bool& pIsTextFile);
+        /* Pointers may be NULL */
+        bool GetNextFile(std::string *short_name, std::string *full_name);
 };
 
 #endif /*DEBUGDUMP_H_*/
diff --git a/lib/Utils/make_descr.cpp b/lib/Utils/make_descr.cpp
index 46dd48c8..c4cc3f35 100644
--- a/lib/Utils/make_descr.cpp
+++ b/lib/Utils/make_descr.cpp
@@ -51,6 +51,9 @@ static void add_content(bool &was_multiline, string& description, const char *he
     }
 }
 
+/* Text attachments smaller than this will be also included in descrition */
+#define INLINE_TEXT_ATT_SIZE 1024
+
 string make_description_bz(const map_crash_report_t& pCrashReport)
 {
     string description;
@@ -77,8 +80,9 @@ string make_description_bz(const map_crash_report_t& pCrashReport)
         const string &filename = it->first;
         const string &type = it->second[CD_TYPE];
         const string &content = it->second[CD_CONTENT];
-        if (type == CD_TXT)
-        {
+        if (type == CD_TXT
+         || (type == CD_ATT && content.size() < INLINE_TEXT_ATT_SIZE)
+        ) {
             if (filename != CD_UUID
              && filename != FILENAME_ARCHITECTURE
              && filename != FILENAME_RELEASE
@@ -87,8 +91,9 @@ string make_description_bz(const map_crash_report_t& pCrashReport)
             ) {
                 add_content(was_multiline, description, filename.c_str(), content.c_str());
             }
+            continue;
         }
-        else if (type == CD_ATT)
+        if (type == CD_ATT)
         {
             add_content(was_multiline, description, "Attached file", filename.c_str());
         }
@@ -172,31 +177,34 @@ string make_description_catcut(const map_crash_report_t& pCrashReport)
 
     for (it = pCrashReport.begin(); it != end; it++)
     {
-        if (it->second[CD_TYPE] == CD_TXT)
+        const string &filename = it->first;
+        const string &type = it->second[CD_TYPE];
+        const string &content = it->second[CD_CONTENT];
+        if (type == CD_TXT)
         {
-            if (it->first != CD_UUID
-             && it->first != FILENAME_ARCHITECTURE
-             && it->first != FILENAME_RELEASE
-             && it->first != CD_REPRODUCE
-             && it->first != CD_COMMENT
+            if (filename != CD_UUID
+             && filename != FILENAME_ARCHITECTURE
+             && filename != FILENAME_RELEASE
+             && filename != CD_REPRODUCE
+             && filename != CD_COMMENT
             ) {
                 pDescription += '\n';
-                pDescription += it->first;
+                pDescription += filename;
                 pDescription += "\n-----\n";
-                pDescription += it->second[CD_CONTENT];
+                pDescription += content;
                 pDescription += "\n\n";
             }
         }
-        else if (it->second[CD_TYPE] == CD_ATT)
+        else if (type == CD_ATT)
         {
             pDescription += "\n\nAttached files\n"
                             "----\n";
-            pDescription += it->first;
+            pDescription += filename;
             pDescription += '\n';
         }
-        else if (it->second[CD_TYPE] == CD_BIN)
+        else if (type == CD_BIN)
         {
-            error_msg(_("Binary file %s will not be reported"), it->first.c_str());
+            error_msg(_("Binary file %s will not be reported"), filename.c_str());
         }
     }
 
diff --git a/src/Daemon/MiddleWare.cpp b/src/Daemon/MiddleWare.cpp
index 66bdea67..bbbca0ac 100644
--- a/src/Daemon/MiddleWare.cpp
+++ b/src/Daemon/MiddleWare.cpp
@@ -64,6 +64,62 @@ static vector_pair_string_string_t s_vectorActionsAndReporters;
 static void RunAnalyzerActions(const char *pAnalyzer, const char *pDebugDumpDir);
 
 
+static char* is_text_file(const char *name, ssize_t *sz)
+{
+    /* We were using magic.h API to check for file being text, but it thinks
+     * that file containing just "0" is not text (!!)
+     * So, we do it ourself.
+     */
+
+    int fd = open(name, O_RDONLY);
+    if (fd < 0)
+        return NULL; /* it's not text (because it does not exist! :) */
+
+    char *buf = (char*)xmalloc(*sz);
+    ssize_t r = *sz = full_read(fd, buf, *sz);
+    close(fd);
+    if (r < 0)
+    {
+        free(buf);
+        return NULL; /* it's not text (because we can't read it) */
+    }
+
+    /* Some files in our dump directories are known to always be textual */
+    if (strcmp(name, "backtrace") == 0
+     || strcmp(name, "cmdline") == 0
+    ) {
+        return buf;
+    }
+
+    /* Every once in a while, even a text file contains a few garbled
+     * or unexpected non-ASCII chars. We should not declare it "binary".
+     */
+    const unsigned RATIO = 50;
+    unsigned total_chars = r + RATIO;
+    unsigned bad_chars = 1; /* 1 prevents division by 0 later */
+    while (--r >= 0)
+    {
+        if (buf[r] >= 0x7f
+         /* among control chars, only '\t','\n' etc are allowed */
+         || (buf[r] < ' ' && !isspace(buf[r]))
+        ) {
+            if (buf[r] == '\0')
+            {
+                /* We don't like NULs very much. Not text for sure! */
+                free(buf);
+                return NULL;
+            }
+            bad_chars++;
+        }
+    }
+
+    if ((total_chars / bad_chars) >= RATIO)
+        return buf; /* looks like text to me */
+
+    free(buf);
+    return NULL; /* it's binary */
+}
+
 /**
  * Transforms a debugdump direcortry to inner crash
  * report form. This form is used for later reporting.
@@ -72,63 +128,69 @@ static void RunAnalyzerActions(const char *pAnalyzer, const char *pDebugDumpDir)
  */
 static void DebugDumpToCrashReport(const char *pDebugDumpDir, map_crash_report_t& pCrashReport)
 {
-    std::string fileName;
-    std::string content;
-    bool isTextFile;
     CDebugDump dd;
     dd.Open(pDebugDumpDir);
-
-    if (!dd.Exist(FILENAME_ARCHITECTURE) ||
-        !dd.Exist(FILENAME_KERNEL) ||
-        !dd.Exist(FILENAME_PACKAGE) ||
-        !dd.Exist(FILENAME_COMPONENT) ||
-        !dd.Exist(FILENAME_RELEASE) ||
-        !dd.Exist(FILENAME_EXECUTABLE))
-    {
-        throw CABRTException(EXCEP_ERROR, "DebugDumpToCrashReport(): One or more of important file(s)'re missing");
+    if (!dd.Exist(FILENAME_ARCHITECTURE)
+     || !dd.Exist(FILENAME_KERNEL)
+     || !dd.Exist(FILENAME_PACKAGE)
+     || !dd.Exist(FILENAME_COMPONENT)
+     || !dd.Exist(FILENAME_RELEASE)
+     || !dd.Exist(FILENAME_EXECUTABLE)
+    ) {
+        throw CABRTException(EXCEP_ERROR, "DebugDumpToCrashReport(): One or more of important file(s) are missing");
     }
 
+    std::string short_name;
+    std::string full_name;
     pCrashReport.clear();
     dd.InitGetNextFile();
-    while (dd.GetNextFile(fileName, content, isTextFile))
+    while (dd.GetNextFile(&short_name, &full_name))
     {
-        //VERB3 log(" file:'%s' text:%d", fileName.c_str(), isTextFile);
-        if (!isTextFile)
+        ssize_t sz = 4*1024;
+        char *text = is_text_file(full_name.c_str(), &sz);
+        if (!text)
         {
             add_crash_data_to_crash_report(pCrashReport,
-                                           fileName,
+                                           short_name,
                                            CD_BIN,
                                            CD_ISNOTEDITABLE,
-                                           concat_path_file(pDebugDumpDir, fileName.c_str())
+                                           full_name
             );
+            continue;
         }
-        else
-        {
-            if (fileName == FILENAME_ARCHITECTURE ||
-                fileName == FILENAME_KERNEL ||
-                fileName == FILENAME_PACKAGE ||
-                fileName == FILENAME_COMPONENT ||
-                fileName == FILENAME_RELEASE ||
-                fileName == FILENAME_EXECUTABLE)
-            {
-                add_crash_data_to_crash_report(pCrashReport, fileName, CD_TXT, CD_ISNOTEDITABLE, content);
-            }
-            else if (fileName != FILENAME_UID &&
-                     fileName != FILENAME_ANALYZER &&
-                     fileName != FILENAME_TIME &&
-                     fileName != FILENAME_DESCRIPTION &&
-                     fileName != FILENAME_REPRODUCE &&
-                     fileName != FILENAME_COMMENT)
-            {
-                if (content.length() < CD_ATT_SIZE)
-                {
-                    add_crash_data_to_crash_report(pCrashReport, fileName, CD_TXT, CD_ISEDITABLE, content);
-                }
-                else
-                {
-                    add_crash_data_to_crash_report(pCrashReport, fileName, CD_ATT, CD_ISEDITABLE, content);
-                }
-            }
+
+        std::string content;
+        if (sz < 4*1024) /* is_text_file did read entire file */
+            content.assign(text, sz);
+        else /* no, need to read it all */
+            dd.LoadText(short_name.c_str(), content);
+        free(text);
+
+        if (short_name == FILENAME_ARCHITECTURE
+         || short_name == FILENAME_KERNEL
+         || short_name == FILENAME_PACKAGE
+         || short_name == FILENAME_COMPONENT
+         || short_name == FILENAME_RELEASE
+         || short_name == FILENAME_EXECUTABLE
+        ) {
+            add_crash_data_to_crash_report(pCrashReport, short_name, CD_TXT, CD_ISNOTEDITABLE, content);
+            continue;
+        }
+
+        if (short_name != FILENAME_UID
+         && short_name != FILENAME_ANALYZER
+         && short_name != FILENAME_TIME
+         && short_name != FILENAME_DESCRIPTION
+         && short_name != FILENAME_REPRODUCE
+         && short_name != FILENAME_COMMENT
+        ) {
+            add_crash_data_to_crash_report(
+                    pCrashReport,
+                    short_name,
+                    (content.length() < CD_ATT_SIZE ? CD_TXT : CD_ATT),
+                    CD_ISEDITABLE,
+                    content
+            );
         }
     }
 }
@@ -139,8 +201,7 @@ static void DebugDumpToCrashReport(const char *pDebugDumpDir, map_crash_report_t
  * @param pDebugDumpDir A debugdump dir containing all necessary data.
  * @return A local UUID.
  */
-static std::string GetLocalUUID(const char *pAnalyzer,
-                                      const char *pDebugDumpDir)
+static std::string GetLocalUUID(const char *pAnalyzer, const char *pDebugDumpDir)
 {
     CAnalyzer* analyzer = g_pPluginManager->GetAnalyzer(pAnalyzer);
     return analyzer->GetLocalUUID(pDebugDumpDir);
diff --git a/src/Daemon/PluginManager.cpp b/src/Daemon/PluginManager.cpp
index 3867ec9b..f8e8fedd 100644
--- a/src/Daemon/PluginManager.cpp
+++ b/src/Daemon/PluginManager.cpp
@@ -58,9 +58,9 @@ bool LoadPluginSettings(const char *pPath, map_plugin_settings_t& pSettings)
         std::string value;
         for (ii = 0; ii < line.length(); ii++)
         {
-            if (line[ii] == '\"')
+            if (line[ii] == '"')
             {
-                in_quote = in_quote == true ? false : true;
+                in_quote = !in_quote;
             }
             if (isspace(line[ii]) && !in_quote)
             {