On Tue, 2010-09-07 at 18:12 +0200, Nikola Pajkovsky wrote:
+static inline void db_row_free(struct db_row *row) +{
- if (!row)
return;
- free(row->db_uuid);
- free(row->db_uid);
- free(row->db_inform_all);
- free(row->db_dump_dir);
- free(row->db_count);
- free(row->db_reported);
- free(row->db_message);
- free(row->db_time);
- free(row);
+}
+static inline void db_list_free(GList *list) +{
- if (!list)
return;
- struct db_row *row = NULL;
- for (GList *li = list; li != NULL; li = g_list_next(li))
- {
row = (struct db_row*)li->data;
db_row_free(row);
- }
- g_list_free(list);
+}
These two functions should go into a new file, database.c
-static void get_table(vector_database_rows_t& pTable,
sqlite3 *db, const char *fmt, ...)
+static GList *vget_table(sqlite3 *db, const char *fmt, va_list p) {
va_list p;
va_start(p, fmt); char *sql = xvasprintf(fmt, p);
va_end(p);
char **table; int ncol, nrow;
@@ -117,39 +113,54 @@ static void get_table(vector_database_rows_t& pTable, int ret = sqlite3_get_table(db, sql, &table, &nrow, &ncol, &err); if (ret != SQLITE_OK) {
string errstr = ssprintf("Error in SQL:'%s' error: %s", sql, err);
error_msg("Error in SQL:'%s' error: %s", sql, err); free(sql); sqlite3_free(err);
throw CABRTException(EXCEP_PLUGIN, errstr.c_str());
}return (GList*)ERR_PTR;
- VERB2 log("%d rows returned by SQL:%s", nrow, sql);
- VERB2 log("%s: %d rows returned by SQL:%s", __func__, nrow, sql); free(sql);
- pTable.clear();
- GList *rows = NULL; int ii; for (ii = 0; ii < nrow; ii++) { int jj;
database_row_t row;
struct db_row *row = (struct db_row*)xzalloc(sizeof(struct db_row)); for (jj = 0; jj < ncol; jj++) { char *val = table[jj + (ncol*ii) + ncol]; switch (jj) {
case 0: row.m_sUUID = val; break;
case 1: row.m_sUID = val; break;
case 2: row.m_sInformAll = val; break;
case 3: row.m_sDebugDumpDir = val; break;
case 4: row.m_sCount = val; break;
case 5: row.m_sReported = val; break;
case 6: row.m_sTime = val; break;
case 7: row.m_sMessage = val; break;
case 0: row->db_uuid = xstrdup(val); break;
case 1: row->db_uid = xstrdup(val); break;
case 2: row->db_inform_all = xstrdup(val); break;
case 3: row->db_dump_dir = xstrdup(val); break;
case 4: row->db_count = xstrdup(val); break;
case 5: row->db_reported = xstrdup(val); break;
case 6: row->db_time = xstrdup(val); break;
case 7: row->db_message = xstrdup(val); break; } }
In old code, if ncol <= 7, then row.m_sMessage was "". Now it will be NULL. This may need to force you to add checks for NULL in many places.
It's easier to check here, *in one place*, that ncol == 8 (or >= 8?) and therefore all fields are non-NULL:
VERB2 log("%s: %d rows returned by SQL:%s", __func__, nrow, sql); if (ncol < 8) error_msg_and_die("Unexpected number of columns: %d", ncol); ... ... const char *val = table[jj + (ncol*ii) + ncol] ? : ""; switch (jj) ...
This way, you know for sure that every field is non-NULL, and the rest of the code need not be changed to check for NULLs.
- return !table.empty();
- if (!table)
return false;
- db_list_free(table);
- return true;
}
I would write it shorter:
db_list_free(table); return (table != NULL);
but your version is ok too.
@@ -188,27 +188,37 @@ int DeleteDebugDump(const char *crash_id, long caller_uid) { CDatabase* database = g_pPluginManager->GetDatabase(g_settings_sDatabase.c_str()); database->Connect();
database_row_t row = database->GetRow(crash_id);
if (row.m_sUUID == "")
struct db_row *row = database->GetRow(crash_id);
if (!row)
return EIO;
if (!row->db_uuid)
Bug. Should be "if (!row->db_uuid[0])"
{ database->DisConnect();
db_row_free(row); return ENOENT; }
@@ -278,21 +278,27 @@ mw_result_t CreateCrashReport(const char *crash_id, { VERB2 log("CreateCrashReport('%s',%ld,result)", crash_id, caller_uid);
- database_row_t row; CDatabase* database = g_pPluginManager->GetDatabase(g_settings_sDatabase.c_str()); database->Connect();
- row = database->GetRow(crash_id);
- struct db_row *row = database->GetRow(crash_id); database->DisConnect();
- if (row.m_sUUID == "")
- if (row && !row->db_uuid) { error_msg("crash '%s' is not in database", crash_id);
}db_row_free(row); return MW_IN_DB_ERROR;
Same bug.
mw_result_t res = FillCrashInfo(crash_id, pCrashData); if (res == MW_OK) { const char *first = get_crash_data_item_content(pCrashData, CD_DUMPDIR).c_str();
if (row.m_sReported == "1")
if (row && row->db_reported[0] == '1')
This is not the same logic, but in this case it is ok (because db_reported is bool).
{ log("Crash is in database already (dup of %s) and is reported", first);
db_row_free(row); return MW_REPORTED; }
if (row.m_sCount != "1")
if (row && row->db_count[0] != '1')
Bug. db_count of "11" will be misinterpreted as "1" here.