From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755877AbcAYJb1 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 25 Jan 2016 04:31:27 -0500
Received: from mailuogwdur.emc.com ([128.221.224.79]:26523 "EHLO
	mailuogwdur.emc.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751188AbcAYJbX convert rfc822-to-8bit (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 25 Jan 2016 04:31:23 -0500
X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd51.lss.emc.com u0P9V8NC026477
X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd51.lss.emc.com u0P9V8NC026477
From: "Singhal, Maneesh" <Maneesh.Singhal@emc.com>
To: Johannes Thumshirn <jthumshirn@suse.de>
CC: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "JBottomley@odin.com" <JBottomley@odin.com>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>
Subject: RE: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client driver
 implementation for  EMC-Symmetrix GuestOS emulated Cut-Through Device
Thread-Topic: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
 driver implementation for  EMC-Symmetrix GuestOS emulated Cut-Through Device
Thread-Index: AdFSsHnhlfOqYluIS12Lfd8RkRvy8AATH2+AAKi7cpAAdxxvAAAKbtyQ
Date: Mon, 25 Jan 2016 09:30:47 +0000
Message-ID: <82807C73E0BDBB498164AAE7B8F6A1481C61AE33@MX105CL01.corp.emc.com>
References: <82807C73E0BDBB498164AAE7B8F6A1481C61263E@MX105CL01.corp.emc.com>
 <20160119160421.GM2742@c203.arch.suse.de>
 <82807C73E0BDBB498164AAE7B8F6A1481C6181A4@MX105CL01.corp.emc.com>
 <20160125092612.GB29561@c203.arch.suse.de>
In-Reply-To: <20160125092612.GB29561@c203.arch.suse.de>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-originating-ip: [10.171.68.96]
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-Sentrion-Hostname: mailusrhubprd52.lss.emc.com
X-RSA-Classifications: public
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thanks Johannes for generously reviewing my patch. It indeed is going to improve the thing a lot. I agree with all your comments so far, and will submit a newer patch soon. 
Since this is my first patch, I have couple of questions regarding submitting the patch:

1.  Is there a page/place where I can see the review comments rather neatly ?, finding out all the comments and infact reading to entire code on text file is cumbersome. If there is no way, then its fine,... I can live with it.
2. When I address the review comments and want to resubmit the patch, do I need to submit the incremental patch or the entire code patch again ? git-format-patch gives me incremental patch only.

Thanks
Maneesh

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn@suse.de]
> Sent: Monday, January 25, 2016 2:56 PM
> To: Singhal, Maneesh
> Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org;
> JBottomley@odin.com; martin.petersen@oracle.com; linux-
> api@vger.kernel.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
> 
> On Sat, Jan 23, 2016 at 05:51:50AM +0000, Singhal, Maneesh wrote:
> > Thanks for your time. My replies inlined...
> >
> 
> [...]
> 
> > > > +		}
> > > > +
> > >
> > > You don't do any cleanup work at
> > > ctd_scsi_response_sanity_check_complete. You could just reutrn 0
> > > here as well.
> > [MS>] Just avoiding multiple exit points from the function
> 
> Please have a look at Documentation/CodingStyle Chapter 7:
> Centralized exiting of functions. Especially this part:
> 
> <quote>
> The goto statement comes in handy when a function exits from
> multiple locations and some common work such as cleanup has to be
> done.  If there is no cleanup needed then just return directly.
> </quote>
> 
> and the example below that section.
> 
> > >
> > > > +
> > > > +		ctd_dprintk_crit(
> 
> [...]
> 
> > > > +	if (request && request->io_timeout < EMCCTD_MAX_RETRY) {
> > >
> > > The following block is a bit long and therefore it's hard to spot an
> > > eventual error of handling the io_mgmt_lock. Can't it be factored
> > > out in a helper function?
> > >
> > [MS>] I know its little longer, but actually fits logically together... Will
> see if it can be factored. Not sure though.
> 
> Yes please. It's not uber important but short functions and paths are
> generally favoured when debugging and reviewing code.
> 
> 
> Thanks,
> 	Johannes
> 
> --
> Johannes Thumshirn                                          Storage
> jthumshirn@suse.de                                +49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG
> Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D
> 2D76 0850

From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Singhal, Maneesh" <Maneesh.Singhal-mb1K0bWo544@public.gmane.org>
Subject: RE: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client driver
 implementation for  EMC-Symmetrix GuestOS emulated Cut-Through Device
Date: Mon, 25 Jan 2016 09:30:47 +0000
Message-ID: <82807C73E0BDBB498164AAE7B8F6A1481C61AE33@MX105CL01.corp.emc.com>
References: <82807C73E0BDBB498164AAE7B8F6A1481C61263E@MX105CL01.corp.emc.com>
 <20160119160421.GM2742@c203.arch.suse.de>
 <82807C73E0BDBB498164AAE7B8F6A1481C6181A4@MX105CL01.corp.emc.com>
 <20160125092612.GB29561@c203.arch.suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Return-path: <linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
In-Reply-To: <20160125092612.GB29561-3LAbnSA0sDC4fIQPS+WK3rNAH6kLmebB@public.gmane.org>
Content-Language: en-US
Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
To: Johannes Thumshirn <jthumshirn-l3A5Bk7waGM@public.gmane.org>
Cc: "linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "JBottomley-wo1vFcy6AUs@public.gmane.org" <JBottomley-wo1vFcy6AUs@public.gmane.org>, "martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org" <martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>, "linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
List-Id: linux-api@vger.kernel.org

Thanks Johannes for generously reviewing my patch. It indeed is going t=
o improve the thing a lot. I agree with all your comments so far, and w=
ill submit a newer patch soon.=20
Since this is my first patch, I have couple of questions regarding subm=
itting the patch:

1.  Is there a page/place where I can see the review comments rather ne=
atly ?, finding out all the comments and infact reading to entire code =
on text file is cumbersome. If there is no way, then its fine,... I can=
 live with it.
2. When I address the review comments and want to resubmit the patch, d=
o I need to submit the incremental patch or the entire code patch again=
 ? git-format-patch gives me incremental patch only.

Thanks
Maneesh

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn-l3A5Bk7waGM@public.gmane.org]
> Sent: Monday, January 25, 2016 2:56 PM
> To: Singhal, Maneesh
> Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> JBottomley-wo1vFcy6AUs@public.gmane.org; martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org; linux-
> api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] drivers/scsi/emcctd: drivers/scsi/emcctd: Client
> driver implementation for EMC-Symmetrix GuestOS emulated Cut-
> Through Device
>=20
> On Sat, Jan 23, 2016 at 05:51:50AM +0000, Singhal, Maneesh wrote:
> > Thanks for your time. My replies inlined...
> >
>=20
> [...]
>=20
> > > > +		}
> > > > +
> > >
> > > You don't do any cleanup work at
> > > ctd_scsi_response_sanity_check_complete. You could just reutrn 0
> > > here as well.
> > [MS>] Just avoiding multiple exit points from the function
>=20
> Please have a look at Documentation/CodingStyle Chapter 7:
> Centralized exiting of functions. Especially this part:
>=20
> <quote>
> The goto statement comes in handy when a function exits from
> multiple locations and some common work such as cleanup has to be
> done.  If there is no cleanup needed then just return directly.
> </quote>
>=20
> and the example below that section.
>=20
> > >
> > > > +
> > > > +		ctd_dprintk_crit(
>=20
> [...]
>=20
> > > > +	if (request && request->io_timeout < EMCCTD_MAX_RETRY) {
> > >
> > > The following block is a bit long and therefore it's hard to spot=
 an
> > > eventual error of handling the io_mgmt_lock. Can't it be factored
> > > out in a helper function?
> > >
> > [MS>] I know its little longer, but actually fits logically togethe=
r... Will
> see if it can be factored. Not sure though.
>=20
> Yes please. It's not uber important but short functions and paths are
> generally favoured when debugging and reviewing code.
>=20
>=20
> Thanks,
> 	Johannes
>=20
> --
> Johannes Thumshirn                                          Storage
> jthumshirn-l3A5Bk7waGM@public.gmane.org                                +49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
> GF: Felix Imend=F6rffer, Jane Smithard, Graham Norton HRB 21284 (AG
> N=FCrnberg) Key fingerprint =3D EC38 9CAB C2C4 F25D 8600 D0D0 0393 96=
9D
> 2D76 0850