[ofa-general] Re: [PATCH] osm: improving error reporting function of the QoS parser

[Yevgeny Kliteynik]

Sasha Khapyorsky wrote:
> Hi Yevgeny,
> 
> On 14:07 Sun 18 Nov     , Yevgeny Kliteynik wrote:
>> [this patch replaces "handle first syntax error in policy file" patch]
>>
>> Improving error reporting function of the QoS parser:
>>  - Making it static
>>  - Printing error message to stderr as well as to the log
>>  - Function now can get formatted string as an argument
>>
>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>> ---
>>  opensm/opensm/osm_qos_parser.y |   22 +++++++++++++++++-----
>>  1 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_qos_parser.y b/opensm/opensm/osm_qos_parser.y
>> index 4738831..bf560aa 100644
>> --- a/opensm/opensm/osm_qos_parser.y
>> +++ b/opensm/opensm/osm_qos_parser.y
>> @@ -47,6 +47,7 @@
>>
>>  #include <stdio.h>
>>  #include <assert.h>
>> +#include <stdarg.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <ctype.h>
>> @@ -125,8 +126,9 @@ static void __parser_add_map_to_port_map(
>>      cl_qmap_t * p_dmap,
>>      cl_map_t  * p_smap);
>>
>> +static void __qos_parser_error(const char *format, ...);
>> +
>>  extern char * __qos_parser_text;
>> -extern void __qos_parser_error (char *s);
>>  extern int __qos_parser_lex (void);
>>  extern FILE * __qos_parser_in;
>>  extern int errno;
>> @@ -1871,14 +1873,24 @@ int __qos_parser_wrap()
>>  /***************************************************
>>   ***************************************************/
>>
>> -void __qos_parser_error (char *s)
>> +static void __qos_parser_error(const char *format, ...)
>>  {
>> +    char s[9999];
> 
> What about to have smaller buffer (let's say 256) and to use vsnprintf()
> below to prevent overflow?

Having smaller buffer won't matter - this function is not used
as log function for errors, but as kind of "fatal" for QoS parser,
so allocating a string only when there is a syntax error in the
policy file doesn't look like an issue to me.
However, vsnprintf() is probably a good idea.

-- Yevgeny

> Sasha
> 
>> +    va_list pvar;
>> +
>>      OSM_LOG_ENTER(p_qos_parser_osm_log, __qos_parser_error);
>> +
>> +    va_start(pvar, format);
>> +    vsprintf(s, format, pvar);
>> +    va_end(pvar);
>> +
>>      osm_log(p_qos_parser_osm_log, OSM_LOG_ERROR,
>>              "__qos_parser_error: ERR AC05: "
>> -            "Syntax error (line %d:%d): %s. "
>> -            "Last text read: \"%s\"\n",
>> -            line_num, column_num, s, __parser_strip_white(__qos_parser_text));
>> +            "Syntax error (line %d:%d): %s",
>> +            line_num, column_num, s);
>> +    fprintf(stderr,
>> +            "Error in QoS Policy File (line %d:%d): %s.\n",
>> +            line_num, column_num, s);
>>      OSM_LOG_EXIT(p_qos_parser_osm_log);
>>  }
>>
>> -- 
>> 1.5.1.4
>>
>>
>