[CRIU] Re: [PATCH 3/4] pr_perror(): print error at the end of line

[Pavel Emelyanov]

On 01/31/2012 11:01 AM, Cyrill Gorcunov wrote:
> On Tue, Jan 31, 2012 at 03:15:07AM +0400, Kir Kolyshkin wrote:
> ...
>>>
>>> +void printk_strerrno(const char *format, ...)
>>> +{
>>> +	char buf[1024];
>>> +	va_list params;
>>> +	int npos;
>>> +
>>> +	va_start(params, format);
>>> +	vsnprintf(buf, sizeof(buf), format, params);
>>> +	va_end(params);
>>> +
>>> +	npos = strlen(buf) - 1;
>>> +	if (buf[npos] == '\n')
>>> +		buf[npos] = '\0';
>>> +	else
>>> +		npos = -1;
>>> +
>>> +	strncat(buf, ": ", sizeof(buf));
>>> +	strncat(buf, strerror(errno), sizeof(buf));
>>> +
>>> +	if (npos != -1)
>>> +		strncat(buf, "\n", sizeof(buf));
>>> +
>>> +	dprintf(get_logfd(), "%s", buf);
>>> +}
>>> +
>>>
>>
>> Well frankly speaking it looks pretty ugly to me:
>> 1. Looks complex.
>> 2. Artificial limit (1024) on resulting string size (could use
>> vasprintf() here)
>> 3. strncat() invocations are inefficient, they scan buf from the
>> beginning till the end every time looking for \0, which you already
>> did.
> 
> yes, but as I say it's not that important if this function is
> called.
> 
>> 4. dprintf() looks like an overkill, what's wrong with write()?
> 
> just to use a pair of vdprintf
> 
>> 5. It doesn't handle the case with multiple \n at the end of format string.
>>
>> Anyway, I came up with what I think is a slightly better version of
>> the above. Still looks ugly to me though:
>> 1. It still looks complex.
>> 2. It still doesn't handle the case with multiple \n at the end.
>>
>> If you and Pavel can tolerate it, let me know and I will redo my
>> patch #3 with this one:
>>
> 
> Looks pretty good to me! Pavel?

OK

> 	Cyrill
> .
>