Patchwork Convert fprintf() to error_request(): virtio-9p

login
register
mail settings
Submitter Sanchit Garg
Date Nov. 30, 2010, 9:52 a.m.
Message ID <20101130095248.8514.75180.stgit@sancgarg.in.ibm.com>
Download mbox | patch
Permalink /patch/73764/
State New
Headers show

Comments

Sanchit Garg - Nov. 30, 2010, 9:52 a.m.
Signed-off-by: Sanchit Garg <sancgarg@linux.vnet.ibm.com>
---
 hw/virtio-9p.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)
Stefan Hajnoczi - Dec. 1, 2010, 9:35 a.m.
On Tue, Nov 30, 2010 at 9:52 AM, Sanchit Garg
<sancgarg@linux.vnet.ibm.com> wrote:
> @@ -3707,19 +3708,19 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>         s->ctx.fs_sm = SM_NONE;
>         s->ctx.xops = none_xattr_ops;
>     } else {
> -        fprintf(stderr, "Default to security_model=none. You may want"
> +        error_report("Default to security_model=none. You may want"
>                 " enable advanced security model using "
>                 "security option:\n\t security_model=passthrough \n\t "
> -                "security_model=mapped\n");
> +                "security_model=mapped");
>         s->ctx.fs_sm = SM_NONE;
>         s->ctx.xops = none_xattr_ops;
>     }

It would be safer to avoid embedded \n\t.  Although I can't find
anything prohibiting it in the source, no other place does this.
Program output is easier to handle when constrained to one message per
line.  Security issues arise when unfiltered inputs are logged *and*
linebreaks are allowed because malicious input can inject fake log
lines.  Let's avoid getting into the habit.

Looks good otherwise.

Stefan
jvrao - Dec. 1, 2010, 6:01 p.m.
On 12/1/2010 1:35 AM, Stefan Hajnoczi wrote:
> On Tue, Nov 30, 2010 at 9:52 AM, Sanchit Garg
> <sancgarg@linux.vnet.ibm.com> wrote:
>> @@ -3707,19 +3708,19 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>>         s->ctx.fs_sm = SM_NONE;
>>         s->ctx.xops = none_xattr_ops;
>>     } else {
>> -        fprintf(stderr, "Default to security_model=none. You may want"
>> +        error_report("Default to security_model=none. You may want"
>>                 " enable advanced security model using "
>>                 "security option:\n\t security_model=passthrough \n\t "
>> -                "security_model=mapped\n");
>> +                "security_model=mapped");
>>         s->ctx.fs_sm = SM_NONE;
>>         s->ctx.xops = none_xattr_ops;
>>     }
> 
> It would be safer to avoid embedded \n\t.  Although I can't find
> anything prohibiting it in the source, no other place does this.
> Program output is easier to handle when constrained to one message per
> line.  Security issues arise when unfiltered inputs are logged *and*
> linebreaks are allowed because malicious input can inject fake log
> lines.  Let's avoid getting into the habit.

Embedded breaks were introduced to give more readable and formatted output.
Stafan do you suggest to print the entire message in one line?

- JV

> 
> Looks good otherwise.
> 
> Stefan
>
Stefan Hajnoczi - Dec. 1, 2010, 9:17 p.m.
On Wed, Dec 1, 2010 at 6:01 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 12/1/2010 1:35 AM, Stefan Hajnoczi wrote:
>> On Tue, Nov 30, 2010 at 9:52 AM, Sanchit Garg
>> <sancgarg@linux.vnet.ibm.com> wrote:
>>> @@ -3707,19 +3708,19 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>>>         s->ctx.fs_sm = SM_NONE;
>>>         s->ctx.xops = none_xattr_ops;
>>>     } else {
>>> -        fprintf(stderr, "Default to security_model=none. You may want"
>>> +        error_report("Default to security_model=none. You may want"
>>>                 " enable advanced security model using "
>>>                 "security option:\n\t security_model=passthrough \n\t "
>>> -                "security_model=mapped\n");
>>> +                "security_model=mapped");
>>>         s->ctx.fs_sm = SM_NONE;
>>>         s->ctx.xops = none_xattr_ops;
>>>     }
>>
>> It would be safer to avoid embedded \n\t.  Although I can't find
>> anything prohibiting it in the source, no other place does this.
>> Program output is easier to handle when constrained to one message per
>> line.  Security issues arise when unfiltered inputs are logged *and*
>> linebreaks are allowed because malicious input can inject fake log
>> lines.  Let's avoid getting into the habit.
>
> Embedded breaks were introduced to give more readable and formatted output.
> Stafan do you suggest to print the entire message in one line?

Yes, exactly.

Stefan
Markus Armbruster - Dec. 14, 2010, 5:33 p.m.
Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Dec 1, 2010 at 6:01 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> On 12/1/2010 1:35 AM, Stefan Hajnoczi wrote:
>>> On Tue, Nov 30, 2010 at 9:52 AM, Sanchit Garg
>>> <sancgarg@linux.vnet.ibm.com> wrote:
>>>> @@ -3707,19 +3708,19 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>>>>         s->ctx.fs_sm = SM_NONE;
>>>>         s->ctx.xops = none_xattr_ops;
>>>>     } else {
>>>> -        fprintf(stderr, "Default to security_model=none. You may want"
>>>> +        error_report("Default to security_model=none. You may want"
>>>>                 " enable advanced security model using "
>>>>                 "security option:\n\t security_model=passthrough \n\t "
>>>> -                "security_model=mapped\n");
>>>> +                "security_model=mapped");
>>>>         s->ctx.fs_sm = SM_NONE;
>>>>         s->ctx.xops = none_xattr_ops;
>>>>     }
>>>
>>> It would be safer to avoid embedded \n\t.  Although I can't find
>>> anything prohibiting it in the source, no other place does this.
>>> Program output is easier to handle when constrained to one message per
>>> line.  Security issues arise when unfiltered inputs are logged *and*
>>> linebreaks are allowed because malicious input can inject fake log
>>> lines.  Let's avoid getting into the habit.
>>
>> Embedded breaks were introduced to give more readable and formatted output.
>> Stafan do you suggest to print the entire message in one line?
>
> Yes, exactly.

If you respin anyway, please fix the subject:
s/error_request/error_report/

Patch

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7c59988..d64d05a 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -18,6 +18,7 @@ 
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-debug.h"
 #include "virtio-9p-xattr.h"
+#include "qemu-error.h"
 
 int debug_9p_pdu;
 
@@ -388,8 +389,8 @@  v9fs_string_alloc_printf(char **strp, const char *fmt, va_list ap)
             len += 1;
             break;
         default:
-            fprintf(stderr,
-		    "v9fs_string_alloc_printf:Incorrect format %c", *iter);
+            error_report(
+           "v9fs_string_alloc_printf:Incorrect format %c", *iter);
             return -1;
         }
         iter++;
@@ -3677,15 +3678,15 @@  VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
 
     if (!fse) {
         /* We don't have a fsdev identified by fsdev_id */
-        fprintf(stderr, "Virtio-9p device couldn't find fsdev with the "
-                "id = %s\n", conf->fsdev_id ? conf->fsdev_id : "NULL");
+        error_report("Virtio-9p device couldn't find fsdev with the "
+                "id = %s", conf->fsdev_id ? conf->fsdev_id : "NULL");
         exit(1);
     }
 
     if (!fse->path || !conf->tag) {
         /* we haven't specified a mount_tag or the path */
-        fprintf(stderr, "fsdev with id %s needs path "
-                "and Virtio-9p device needs mount_tag arguments\n",
+        error_report("fsdev with id %s needs path "
+                "and Virtio-9p device needs mount_tag arguments",
                 conf->fsdev_id);
         exit(1);
     }
@@ -3707,19 +3708,19 @@  VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
         s->ctx.fs_sm = SM_NONE;
         s->ctx.xops = none_xattr_ops;
     } else {
-        fprintf(stderr, "Default to security_model=none. You may want"
+        error_report("Default to security_model=none. You may want"
                 " enable advanced security model using "
                 "security option:\n\t security_model=passthrough \n\t "
-                "security_model=mapped\n");
+                "security_model=mapped");
         s->ctx.fs_sm = SM_NONE;
         s->ctx.xops = none_xattr_ops;
     }
 
     if (lstat(fse->path, &stat)) {
-        fprintf(stderr, "share path %s does not exist\n", fse->path);
+        error_report("share path %s does not exist", fse->path);
         exit(1);
     } else if (!S_ISDIR(stat.st_mode)) {
-        fprintf(stderr, "share path %s is not a directory \n", fse->path);
+        error_report("share path %s is not a directory", fse->path);
         exit(1);
     }