Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver

From: Dan Carpenter <dan.carpenter@oracle.com>
To: KY Srinivasan <kys@microsoft.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"ohering@suse.com" <ohering@suse.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"virtualization@lists.osdl.org" <virtualization@lists.osdl.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	"devel@linuxdriverproject.org" <devel@linuxdriverproject.org>
Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
Date: Fri, 16 Mar 2012 17:26:49 +0300	[thread overview]
Message-ID: <20120316142649.GJ3163@mwanda> (raw)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481B76671F@TK5EX14MBXC126.redmond.corp.microsoft.com>

[-- Attachment #1.1: Type: text/plain, Size: 1704 bytes --]

On Fri, Mar 16, 2012 at 01:14:07PM +0000, KY Srinivasan wrote:
> Dan,
> I am trying to understand your concern here:
> You will agree with me that the current code does not overflow the 
> buffer since the output is limited to MAX bytes and that is how 
> big the output buffer is sized. Now, as I pointed out earlier, if the
> string to be converted were to fully occupy the MAX bytes or even be
> larger than MAX bytes (no buffer overflow here since we limit the
> conversion to MAX bytes), we will get a malformed packet that will be
> sent to the host since the size field of the message would exceed
> the protocol specified size limit. I was merely pointing out that in
> this case, I would rather have the host reject the message than send
> a truncated Key/Value string (if we were to ever get invalid key or
> value strings). 
> 

Sending malformed packets is a bad idea.

Normally, if you handle the error as close to the cause of the error
as possible then it makes everything easier to read.  In this case
especially, it's so easy to catch any errors.  If you decide to go
ahead and send the malformed message, at least put a comment.

When I read code, I spend all my time looking for what it does
wrong.  So when code doesn't have any error handling and sets
keylen = -EINVAL then sure, it's fewer lines of code to read, but
it doesn't make my life easier.  Readable code has obvious error
handling.

Introducing off by one errors is an especially bad idea.  It doesn't
matter how harmless they are, because they end up getting copy and
pasted.  I don't know how to make it any clearer than that.  :/
Never never never do that!

regards,
dan carpenter

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel