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

Kir Kolyshkin kir at openvz.org
Mon Jan 30 18:15:07 EST 2012


On 01/30/2012 10:32 PM, Cyrill Gorcunov wrote:
> On Mon, Jan 30, 2012 at 09:57:12PM +0400, Pavel Emelyanov wrote:
>> On 01/30/2012 09:25 PM, Cyrill Gorcunov wrote:
>>> On Mon, Jan 30, 2012 at 09:18:39PM +0400, Kir Kolyshkin wrote:
>>>> This is a standard convention to print error message (i.e. strerror(errno))
>>>> at the end of line, like this:
>>>>
>>>> 	Cannot remove file: Permission denied
>>>>
>>>> So pr_perror is fixed to follow this convention (using GNU extension
>>>> %m helps a lot here). Unfortunately, due to this we have to make
>>>> pr_perror() print a new line character, too, so we had to strip it
>>>> from the all pr_perror() invocations.
>>>>
>>>> Signed-off-by: Kir Kolyshkin<kir at openvz.org>
>>>> ---
>>> Hi Kir, actually I don't like it, we have a number of pr_ helpers
>>> (the idea in first place was to be similar to pr_ helpers used
>>>   in kernel) so this pr_perror become a special one from the whole
>>> pr_ helpers set in terms of "implicit new line at the end".
>>>
>>> Kir, can we somehow bring back "explicit \n" at the end somehow?
>> I have the same question.
>>
>> If it requires some non-trivial things to code, then I don't insist, let's
>> live with it and
>>
>> Acked-by: Pavel Emelyanov<xemul at parallels.com>
>>
> Kir, might be something like below?
>
> 	Cyrill
> ---
> From: Cyrill Gorcunov<gorcunov at openvz.org>
> Date: Mon, 30 Jan 2012 22:30:52 +0400
> Subject: [PATCH] util: Modify pr_perror to follow perror convention
>
> Ie to print system error message after all arguments
> caller passed, for example
>
>   | Error (crtools.c:249): Can't open '~/testing': No such file or directory
>
> preserving new line if it was present in caller format.
>
> Signed-off-by: Cyrill Gorcunov<gorcunov at openvz.org>
> ---
>   include/util.h |    8 ++------
>   util.c         |   25 +++++++++++++++++++++++++
>   2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/include/util.h b/include/util.h
> index efe1b03..7198cde 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -17,6 +17,7 @@
>   #include "types.h"
>
>   extern void printk(const char *format, ...);
> +extern void printk_strerrno(const char *format, ...);
>
>   #define PREF_SHIFT_OP(pref, op, size)	((size) op (pref ##BYTES_SHIFT))
>   #define KBYTES_SHIFT	10
> @@ -35,6 +36,7 @@ extern void printk(const char *format, ...);
>   #define pr_err(fmt, ...)	printk("Error (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
>   #define pr_panic(fmt, ...)	printk("PANIC (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
>   #define pr_warning(fmt, ...)	printk("Warning (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> +#define pr_perror(fmt, ...)	printk_strerrno("Error (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
>
>   #define pr_err_jmp(label)					\
>   	do {							\
> @@ -81,12 +83,6 @@ extern void printk(const char *format, ...);
>   		exit(1);					\
>   	} while (0)
>
> -#define pr_perror(fmt, ...)					\
> -	do {							\
> -		pr_err("%s: " fmt,  strerror(errno),		\
> -			##__VA_ARGS__);				\
> -	} while (0)
> -
>   #ifndef BUG_ON_HANDLER
>
>   #ifdef CR_NOGLIBC
> diff --git a/util.c b/util.c
> index de22151..1bff4a7 100644
> --- a/util.c
> +++ b/util.c
> @@ -47,6 +47,31 @@ void printk(const char *format, ...)
>   	va_end(params);
>   }
>
> +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.
4. dprintf() looks like an overkill, what's wrong with write()?
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:

diff --git a/util.c b/util.c
index de22151..6d7a213 100644
--- a/util.c
+++ b/util.c
@@ -38,12 +38,34 @@

  #include "crtools.h"

+static void vprintk(const char *format, va_list params)
+{
+	vdprintf(get_logfd(), format, params);
+}
+
  void printk(const char *format, ...)
  {
  	va_list params;

  	va_start(params, format);
-	vdprintf(get_logfd(), format, params);
+	vprintk(format, params);
+	va_end(params);
+}
+
+void printk_strerror(const char *format, ...)
+{
+	va_list params;
+	int n = strlen(format);
+	char *buf = alloca(n + sizeof(": %m"));
+
+	strcpy(buf, format);
+	if (format[n - 1] == '\n')
+		strcpy(buf + n - 1, ": %m\n");
+	else
+		strcpy(buf + n, ": %m");
+
+	va_start(params, format);
+	vprintk(buf, params);
  	va_end(params);
  }
  


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://openvz.org/pipermail/criu/attachments/20120131/8c4047cf/attachment.html


More information about the CRIU mailing list