From: KY Srinivasan <kys@microsoft.com>
To: Levente Kurusa <levex@linux.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
        "olaf@aepfle.de" <olaf@aepfle.de>,
        "apw@canonical.com" <apw@canonical.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>
Subject: RE: [PATCH V3 1/1] Drivers: hv: Implement the file copy service
Thread-Topic: [PATCH V3 1/1] Drivers: hv: Implement the file copy service
Thread-Index: AQHPFtTw0Ur+cw3x2keGd11Wuxh+LJqPiKmAgAAB7IA=
Date: Tue, 21 Jan 2014 19:09:38 +0000
Message-ID: <94f51d7bf34545e5b96fb5860fe8f339@BY2PR03MB299.namprd03.prod.outlook.com>
References: <1390331164-14292-1-git-send-email-kys@microsoft.com>
 <52DEC3FD.2040701@linux.com>
In-Reply-To: <52DEC3FD.2040701@linux.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org


> -----Original Message-----
> From: Levente Kurusa [mailto:ilevex.linux@gmail.com] On Behalf Of Levente
> Kurusa
> Sent: Tuesday, January 21, 2014 11:01 AM
> To: KY Srinivasan; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH V3 1/1] Drivers: hv: Implement the file copy service
> 
> Hello,
> 
> On 01/21/2014 08:06 PM, K. Y. Srinivasan wrote:
> > Implement the file copy service for Linux guests on Hyper-V. This permits the
> > host to copy a file (over VMBUS) into the guest. This facility is part of
> > "guest integration services" supported on the Windows platform.
> > Here is a link that provides additional details on this functionality:
> >
> > http://technet.microsoft.com/en-us/library/dn464282.aspx
> >
> > In V1 version of the patch I have addressed comments from
> > Olaf Hering <olaf@aepfle.de> and Dan Carpenter
> <dan.carpenter@oracle.com>
> >
> > In V2 version of this patch I did some minor cleanup (making some globals
> > static). In this version of the patch I have addressed all of Olaf's
> > most recent set of comments/concerns.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> 
> Just a few comments.
> 
> > +	/*
> > +	 * The  strings sent from the host are encoded in
> > +	 * in utf16; convert it to utf8 strings.
> > +	 * The host assures us that the utf16 strings will not exceed
> > +	 * the max lengths specified. We will however, reserve room
> > +	 * for the string terminating character - in the utf16s_utf8s()
> > +	 * function we limit the size of the buffer where the converted
> > +	 * string is placed to W_MAX_PATH -1 to gaurantee
> 
> 	s/gaurantee/guarantee
I will fix this.
> 
> > +	 * that the strings can be properly terminated!
> > +	 */
> > +
> > +	switch (operation) {
> > +	case START_FILE_COPY:
> > +		memset(smsg_out, 0, sizeof(struct hv_start_fcopy));
> > +		smsg_out->hdr.operation = operation;
> > +		smsg_in = (struct hv_start_fcopy
> *)fcopy_transaction.fcopy_msg;
> > +
> > +		utf16s_to_utf8s((wchar_t *)smsg_in->file_name,
> W_MAX_PATH,
> > +				UTF16_LITTLE_ENDIAN,
> > +				(__u8 *)smsg_out->file_name, W_MAX_PATH -
> 1);
> > +
> > +		utf16s_to_utf8s((wchar_t *)smsg_in->path_name,
> W_MAX_PATH,
> > +				UTF16_LITTLE_ENDIAN,
> > +				(__u8 *)smsg_out->path_name, W_MAX_PATH -
> 1);
> > +
> > +		smsg_out->copy_flags = smsg_in->copy_flags;
> > +		smsg_out->file_size = smsg_in->file_size;
> > +		break;
> > +
> > +	default:
> > +		break;
> > +	}
> > +	up(&fcopy_transaction.read_sema);
> > +	return;
> > +}
> > +
> > [...]
> > +static ssize_t fcopy_read(struct file *file, char __user *buf,
> > +		size_t count, loff_t *ppos)
> > +{
> > +	int ret;
> > +	void *src;
> > +	size_t copy_size;
> > +	int operation;
> > +
> > +	/*
> > +	 * Wait until there is something to be read.
> > +	 */
> > +	ret = down_interruptible(&fcopy_transaction.read_sema);
> > +	if (ret)
> > +		return -EINTR;
> 
> I don't know, but since you use 'ret' only once and you don't
> even read it after, you could just simply remove it and check
> the return code of down_interruptible() directly

Good point; I will fix this.

Thank you,

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