Patchwork virtio-serial: Fix segfault on guest boot

login
register
mail settings
Submitter Amit Shah
Date June 22, 2011, 4:53 a.m.
Message ID <20110622045330.GB5789@amit-x200.redhat.com>
Download mbox | patch
Permalink /patch/101391/
State New
Headers show

Comments

Amit Shah - June 22, 2011, 4:53 a.m.
On (Sun) 19 Jun 2011 [00:43:20], Blue Swirl wrote:
> On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:
> >
> >> > >          if (!cpkt.value) {
> >> > > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> >> > > -                         vser->bus.qbus.name);
> >> > > -            break;
> >> > > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> >> > > +            return;
> >> >
> >> > The line split should remain -- else it goes beyond 80 chars.
> >>
> >> It's already beyond 80 chars to me.
> >
> > I prefer to not break strings that get printed out -- makes it easier
> > for greppers to find out the source of the string.
> 
> Please read CODING_STYLE and use scripts/checkpatch.pl before
> submitting patches.

Good sense should dictate what goes in CODING_STYLE, not the other way
around.

From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
From: Amit Shah <amit.shah@redhat.com>
Date: Wed, 22 Jun 2011 10:20:48 +0530
Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit

Output that's logged can be beyond 80 chars to preserve sane grepping.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 CODING_STYLE |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Blue Swirl - June 26, 2011, 5:43 p.m.
On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Sun) 19 Jun 2011 [00:43:20], Blue Swirl wrote:
>> On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote:
>> > On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:
>> >
>> >> > >          if (!cpkt.value) {
>> >> > > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
>> >> > > -                         vser->bus.qbus.name);
>> >> > > -            break;
>> >> > > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
>> >> > > +            return;
>> >> >
>> >> > The line split should remain -- else it goes beyond 80 chars.
>> >>
>> >> It's already beyond 80 chars to me.
>> >
>> > I prefer to not break strings that get printed out -- makes it easier
>> > for greppers to find out the source of the string.
>>
>> Please read CODING_STYLE and use scripts/checkpatch.pl before
>> submitting patches.
>
> Good sense should dictate what goes in CODING_STYLE, not the other way
> around.

Unfortunately there is huge amount of bike shedding in this area,
there is no One Good Sense but several which conflict with each other.
Discussions about changing CODING_STYLE haven't been very fruitful.

> From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
> Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
> From: Amit Shah <amit.shah@redhat.com>
> Date: Wed, 22 Jun 2011 10:20:48 +0530
> Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
>
> Output that's logged can be beyond 80 chars to preserve sane grepping.

Nack. Grepping (why just logs?) is just one use case. There are other
cases where shorter line length is preferred, like editors, terminals,
patch generation, mail systems munging long lines etc.

>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  CODING_STYLE |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 5ecfa22..886221c 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -31,7 +31,10 @@ Do not leave whitespace dangling off the ends of lines.
>
>  2. Line width
>
> -Lines are 80 characters; not longer.
> +Lines are 80 characters; not longer.  An exception is for output that
> +is logged, for example via fprintf() / error_report() functions,
> +making it straightforward for people to grep the code for the source
> +of the output.
>
>  Rationale:
>  - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
> --
> 1.7.5.4
>
>
>
>                Amit
>
Amit Shah - June 27, 2011, 9:48 a.m.
On (Sun) 26 Jun 2011 [20:43:16], Blue Swirl wrote:
> On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:

[snip]

> > From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
> > Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
> > From: Amit Shah <amit.shah@redhat.com>
> > Date: Wed, 22 Jun 2011 10:20:48 +0530
> > Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
> >
> > Output that's logged can be beyond 80 chars to preserve sane grepping.
> 
> Nack. Grepping (why just logs?) is just one use case. There are other

I'm not talking about grepping logs.  I'm talking about grepping code
once you have something in the logs.  With line breaks in the code but
not in the log, you will not get the desired result.

Example:


fprintf("This is a line ");
fprintf("broken in two code lines\n");


Output is:

This is a line broken in two code lines


Picking that line from the output and grepping for it in the source
doesn't get you what you want.

		Amit
Blue Swirl - July 1, 2011, 7:43 p.m.
On Mon, Jun 27, 2011 at 12:48 PM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Sun) 26 Jun 2011 [20:43:16], Blue Swirl wrote:
>> On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:
>
> [snip]
>
>> > From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
>> > Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
>> > From: Amit Shah <amit.shah@redhat.com>
>> > Date: Wed, 22 Jun 2011 10:20:48 +0530
>> > Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
>> >
>> > Output that's logged can be beyond 80 chars to preserve sane grepping.
>>
>> Nack. Grepping (why just logs?) is just one use case. There are other
>
> I'm not talking about grepping logs.  I'm talking about grepping code
> once you have something in the logs.  With line breaks in the code but
> not in the log, you will not get the desired result.
>
> Example:
>
>
> fprintf("This is a line ");
> fprintf("broken in two code lines\n");
>
>
> Output is:
>
> This is a line broken in two code lines
>
>
> Picking that line from the output and grepping for it in the source
> doesn't get you what you want.

Nack. Grep would not match for example
fprintf("'%s' invalid media\n", s);
unless you know how to grep only for some parts of the error string
and in that case splitting the line would not hurt very much anymore.

If you want robust grepping, each error message should contain an ID,
for example:
fprintf("ERROR-ID-0015:'%s' invalid media\n", s);

Then grepping would work even for
fprintf(
"ERROR-ID-0015:"
"'%s'"
" invalid"
" media\n",
s);
Stefan Hajnoczi - July 2, 2011, 8:38 a.m.
On Fri, Jul 1, 2011 at 8:43 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jun 27, 2011 at 12:48 PM, Amit Shah <amit.shah@redhat.com> wrote:
>> On (Sun) 26 Jun 2011 [20:43:16], Blue Swirl wrote:
>>> On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:
>>
>> [snip]
>>
>>> > From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
>>> > Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
>>> > From: Amit Shah <amit.shah@redhat.com>
>>> > Date: Wed, 22 Jun 2011 10:20:48 +0530
>>> > Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
>>> >
>>> > Output that's logged can be beyond 80 chars to preserve sane grepping.
>>>
>>> Nack. Grepping (why just logs?) is just one use case. There are other
>>
>> I'm not talking about grepping logs.  I'm talking about grepping code
>> once you have something in the logs.  With line breaks in the code but
>> not in the log, you will not get the desired result.
>>
>> Example:
>>
>>
>> fprintf("This is a line ");
>> fprintf("broken in two code lines\n");
>>
>>
>> Output is:
>>
>> This is a line broken in two code lines
>>
>>
>> Picking that line from the output and grepping for it in the source
>> doesn't get you what you want.
>
> Nack. Grep would not match for example
> fprintf("'%s' invalid media\n", s);
> unless you know how to grep only for some parts of the error string
> and in that case splitting the line would not hurt very much anymore.
>
> If you want robust grepping, each error message should contain an ID,
> for example:
> fprintf("ERROR-ID-0015:'%s' invalid media\n", s);
>
> Then grepping would work even for
> fprintf(
> "ERROR-ID-0015:"
> "'%s'"
> " invalid"
> " media\n",
> s);

I don't see split lines as an issue because I never grep for an entire
line.  Pick a small, unique, fixed part of the message and you'll find
it.

"Small" in order to avoid any formatting or split line issues.
"Unique" in order to cut down the number of grep results.
"Fixed" in order to avoid format string expansions as Blue Swirl mentioned.

Stefan
Amit Shah - July 4, 2011, 6:05 a.m.
On (Sat) 02 Jul 2011 [09:38:30], Stefan Hajnoczi wrote:

...

> I don't see split lines as an issue because I never grep for an entire
> line.  Pick a small, unique, fixed part of the message and you'll find
> it.
> 
> "Small" in order to avoid any formatting or split line issues.

Unless you pick your Small string that was split across multiple lines.

> "Unique" in order to cut down the number of grep results.
> "Fixed" in order to avoid format string expansions as Blue Swirl mentioned.

		Amit

Patch

diff --git a/CODING_STYLE b/CODING_STYLE
index 5ecfa22..886221c 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -31,7 +31,10 @@  Do not leave whitespace dangling off the ends of lines.
 
 2. Line width
 
-Lines are 80 characters; not longer.
+Lines are 80 characters; not longer.  An exception is for output that
+is logged, for example via fprintf() / error_report() functions,
+making it straightforward for people to grep the code for the source
+of the output.
 
 Rationale:
  - Some people like to tile their 24" screens with a 6x4 matrix of 80x24