linux-kernel - Re: [KVM PATCH v9 1/2] KVM: make io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 8 Jul 2009 10:47:42 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Gregory Haskins <ghaskins@...ell.com>
Cc:	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, avi@...hat.com
Subject: Re: [KVM PATCH v9 1/2] KVM: make io_bus interface more robust

On Tue, Jul 07, 2009 at 01:26:11PM -0400, Gregory Haskins wrote:
> These patches pass checkpatch.pl and I happen to like the extra
> whitespace for readability.  I agree that a random isolated whitespace
> hunk, or double whitespace in a row are probably inadvertent and should
> be pointed out.  But these little one liners in the middle of code I
> generally do on purpose (for instance, [A]).

Where it's on purpose, it's on purpose. I am just trying to convey a
rather obvious point that each line of code should have a purpose in
life, and that includes empty lines :)

> I suppose its personal preference either way, so I guess unless Avi
> objects lets just each have our own style in that regard.

I think Avi already said we don't need to standardize everything.
I hope at least some of the comments were helpful.

> >> @@ -552,6 +554,11 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
> >>  	 * Initialize PIO device
> >>  	 */
> >>  	kvm_iodevice_init(&s->dev, &picdev_ops);
> >> -	kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
> >> +	ret = kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
> >> +	if (ret < 0) {
> >>     
> >
> > I thought the function returns 0 on success?
> > If so can we just if (ret) all over?
> >
> >   
> 
> I guess, but what does that churn buy us?

It's not that important really. I think we need to document the return
value, and check it according to how it is documented. The reason I
commented, I see < 0 and ask "what if it is > 0"? I look it up and it
turns out it's never > 0.  So why check < 0?

> >> +	ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev);
> >> +	if (ret < 0)
> >> +		kfree(dev);
> >> +
> >>     
> >
> > kill empty line
> >   
> 
> Why do you object here especially when there is precedence

How do you mean, precedence?

> with
> something like the space before the return with [B]?  I think big
> mono-blocks of code are ugly and harder to read, personally.

I don't intend to keep arguing and I agree it's a question of style.
But since you ask why I'll try to answer. I think an
empty line should help delimit a block of code with some common meaning,
like a paragraph.  But if overused it loses meaning and stops being
helpful.

E.g., it does not make sense to put it between every 2 lines of code in a
function.  It also does not make sense to put it after } which is
already delimiting a block in a visible way; it does not make sense to
put it around a multiline comment which is delimited by /**/.  It does
not make sense to put it around an idented block which is already
delimited by indentation.

> >> +		if (bus->devs[i] == dev) {
> >> +			bus->devs[i] = bus->devs[--bus->dev_count];
> >> +			break;
> >> +		}
> >> +	}
> >>     
> >
> > no {} around single statement

This is actually part of style IMO, it is just hard for a perl script to
catch :).

> >
> >   
> >>  }
> >>  
> >>  static struct notifier_block kvm_cpu_notifier = {
> >>     
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/