On Wednesday 30 of May 2012 15:14:23 Nikola Pajkovsky wrote:
> Denys Vlasenko <dvlasenk@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"