On Wednesday 30 of May 2012 15:14:23 Nikola Pajkovsky wrote:
Denys Vlasenko <dvlasenk(a)redhat.com> writes:
> On 05/30/2012 12:37 PM, Jakub Filak wrote:
>> +static bool problem_condition_evaluate_and(struct dump_dir *dd,
>> + const struct
>> problem_condition *const *condition) +{
>> + /* We stop on the first FALSE condition */
>> + while (*condition != NULL)
>> + {
>> + const struct problem_condition *c = *condition;
>> + char *field_data = dd_load_text(dd, c->field_name);
>> + bool value = c->evaluate(field_data, c->args);
>
> Note that field_data is never NULL... (see below).
>
>> +static bool time_interval_problem_condition(const char *field_data,
>> const void* args) +{
>> + if (!field_data)
>> + return FALSE;
>
> Never happens.
>
>> + const struct time_interval *const interval = (const struct
>> time_interval *)args; + const time_t timestamp = atol(field_data);
>> +
>> + return interval->from<= timestamp && timestamp <=
interval->to;
>
> Why timespamp == 0 is treated specially?
> If there is a reason, it should be explained in a comment somewhere.
>
>> +static bool equal_string_problem_condition(const char *field_data, const
>> void* args) +{
>> + return (field_data == NULL && args == NULL)
>
> Never happens.
>
>> + || (field_data != NULL
>
> Always true
>
> && !strcmp(field_data, (const char *)args));
>>
>> +static bool substring_problem_condition(const char *field_data, const
>> void* args) +{
>> + /* BEWARE : formally empty string IS ALWAYS substring of any string
>> */
>> + /* but it is NOT VALID in our case */
>> + return field_data != NULL
>
> field_data is always !NULL
>
> && strstr(field_data, (const char *)args);
>>
>> +static void initialize_element_condition(struct problem_condition
>> *condition, + const char
>> *element,
>> + const char *value)
>> +{
>> + condition->field_name = element;
>> + condition->args = value;
>> +
>> + /* use substring evaluate function for CGROUP element */
>> + if (strcmp(element, FILENAME_CGROUP) == 0)
>> + condition->evaluate = substring_problem_condition;
>> + else
>> + /* an evaluate function is string equality by default */
>> + condition->evaluate = equal_string_problem_condition;
>> +}
>
> I'm not that familiar with cgroups. Why we do substring match?
> How cgroup names look like? Maybe add a comment with an example...
>
>> + g_variant_get(parameters,
>>
"(ssttb)",&element,&value,×tamp_from,×tamp_to,&all);
+
>> + if (strlen(element) == 0)
>
> "if (element[0])" is more efficient check.
>
>> + {
>> + g_dbus_method_invocation_return_dbus_error(invocation,
>> +
>> "org.freedesktop.problems.InvalidElement", +
>> _("Empty Element Name")); +
return;
>> + }
>
> I don't understand why you don't allow empty names.
> Granted, they don't make sense, but so do names like
"\/\/\\//\陰莖增大增***",
> but we don't check for that, right?
> I propose not checking for "".
>
>> +
>> + if (all&& polkit_check_authorization_dname(caller,
>> "org.freedesktop.problems.getall") == PolkitYes) +
caller_uid
>> = 0;
>> +
>> + /* FIXME : Temporary until we decide how to handle this */
>> + if (!g_settings_dump_location)
>> + g_settings_dump_location = (char*)"/var/spool/abrt";
>
> g_settings_dump_location must be a malloced string, or else:
g_settings_dump_location is always set to location from config or
malloc'ed "/var/spool/abrt" string if abrt_load_conf() is called
snip from abrt/src/lib/abrt_conf.c:65
g_hash_table_remove(settings, "DumpLocation");
}
else
g_settings_dump_location = xstrdup("/var/spool/abrt"); <===== HERE
GHashTableIter ite
> void free_abrt_conf_data()
> {
>
> free(g_settings_sWatchCrashdumpArchiveDir);
> g_settings_sWatchCrashdumpArchiveDir = NULL;
>
> free(g_settings_dump_location); <============= BOOM
> g_settings_dump_location = NULL;
>
> }
abrt_load_conf() nor free_abrt_conf_data() are never called in abrt-dbus.c
file
Therefore I wrote the following comment:
/* FIXME : Temporary until we decide how to handle this */
I'm not sure why we don't load ABRT configuration.
So, I'm going to use this solution:
g_settings_dump_location ? g_settings_dump_location : "/var/spool/abrt"
>
>> +
>> + response = get_problem_dirs_for_element_in_time(caller_uid,
>> + element,
>> + value,
>> + timestamp_from,
>> + timestamp_to,
>> +
>> g_settings_dump_location);>
> You can simply add that bit here instead:
>
> g_settings_dump_location ? g_settings_dump_location : "/var/spool/abrt"