lkml.org 
[lkml]   [2012]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Friday, March 16, 2012 1:46 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > Alan Stern
> > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > messages in the driver
> >
> > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > /*
> > > * The windows host expects the key/value pair to be encoded
> > > * in utf16.
> > > */
> > > keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > UTF16_HOST_ENDIAN,
> > > - (wchar_t *) kvp_data->data.key,
> > > + (wchar_t *) kvp_data->key,
> > > HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +
> >
> > I feel like a jerk for asking this, but is the output length correct
> > here? It seems like we could go over again. Also utf8s_to_utf16s()
> > can return negative error codes, why do we ignore those?
>
> We are returning the strings back to the host here. There are checks elsewhere
> in the code to ensure that all strings we return to the host can be accommodated
> in the available space. For the most part these are strings that the host gave us in the
> first place that have already been validated. Furthermore, there are checks on the
> host side to ensure that the returned size parameters are consistent with the protocol
> definitions for the key value pair. For instance let us say somehow we got into a
> situation where the converted utf16 string occupied the entire MAX sized array
> without any room for the terminating character and we set the length parameter
> to 2 more than the MAX value as this code would do. The host would simply discard the
> message as an illegal message. This would be more appropriate than sending a
> truncated key or value.
>

Uh... Looking at it again, this code is clearly off by one. If
we're not going to hit the limit, then we're not going to truncate,
so that's not a concern. Let's just use the correct limit here.

The problem is that off-by-ones tend to reproduce by copy and paste.
It's best to never introduce any, even harmless ones.

Either that or add a comment. /* Don't care about wrong limitter
because we trust the input. */.
> With regards to the negative values, negative values indicate a failure of some sort
> in the conversion. Since the host is the recipient here, host will correctly deal with the
> transaction by discarding the tuple.

I'm not super familiar with this subsystem. Where can I find code
for rejecting bad transactions? It seems like an easy thing to
handle the error in both places. It makes auditing the code a lot
simpler.

regards,
dan carpenter

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-03-16 08:19    [W:0.076 / U:0.868 seconds]
©2003-2014 Jasper Spaans. Advertise on this site