Re: [PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query

[Jim Cromie]

On Mon, Jul 11, 2011 at 12:36 PM, Bart Van Assche <bvanassche@xxxxxxx> wrote:
> On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@xxxxxxxxx> wrote:
>> Will reuse for show_pending_query too.  Alloc and free
>> print buffer space inside ddebug_exec_queries, instead of
>> a permanent static allocation.
>>
>> Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
>> ---
>>  lib/dynamic_debug.c |   24 +++++++++++++++++++-----
>>  1 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>> index de2a679..81268e2 100644
>> --- a/lib/dynamic_debug.c
>> +++ b/lib/dynamic_debug.c
>> @@ -96,6 +96,19 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
>>        return buf;
>>  }
>>
>> +static char *prbuf_query;
>> +
>> +static char *show_ddebug_query(const struct ddebug_query *q)
>> +{
>> +       sprintf(prbuf_query,
>> +               "q->function=\"%s\" q->filename=\"%s\" "
>> +               "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u",
>> +               q->function, q->filename, q->module, q->format,
>> +               q->first_lineno, q->last_lineno);
>> +
>> +       return prbuf_query;
>> +}
>> +
>>  /*
>>  * Search the tables for _ddebug's which match the given
>>  * `query' and apply the `flags' and `mask' to them.  Tells
>> @@ -344,11 +357,7 @@ static int ddebug_parse_query(char *words[], int nwords,
>>        }
>>
>>        if (verbose)
>> -               pr_info("q->function=\"%s\" q->filename=\"%s\" "
>> -                       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
>> -                       query->function, query->filename,
>> -                       query->module, query->format, query->first_lineno,
>> -                       query->last_lineno);
>> +               pr_info("parsed %s\n", show_ddebug_query(query));
>>
>>        return 0;
>>  }
>> @@ -440,6 +449,10 @@ static int ddebug_exec_queries(char *query)
>>        char *split;
>>        int i, errs = 0, exitcode = 0, rc;
>>
>> +       prbuf_query = kmalloc(1024, GFP_KERNEL);
>> +       if (prbuf_query == NULL)
>> +               return -ENOMEM;
>> +
>>        for (i = 0; query; query = split, i++) {
>>                split = strchr(query, ';');
>>                if (split)
>> @@ -454,6 +467,7 @@ static int ddebug_exec_queries(char *query)
>>                        exitcode = rc;
>>                }
>>        }
>> +       kfree(prbuf_query);
>>        if (verbose)
>>                pr_info("processed %d queries, with %d errs\n", i, errs);
>
> Why to invoke kmalloc() with a magic constant as the first argument
> while the above code can be simplified by replacing the sprintf() call
> (that is missing an output buffer size check) by a kasprintf() call ?
> And why does the static variable 'prbuf_query' exist at all ?
>
> Bart.
>

using kasprintf would entail;
- a local char* var,
- a call to the string-producer (show_ddebug_query above)
- a kfree(var)
all in the caller context.
Using a preallocated string buffer avoids this,
and is slightly more efficient wrt kallocs.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/