linux-kernel - Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen

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] [day] [month] [year] [list]
Message-ID: <20110806143950.GA29514@dumpdata.com>
Date:	Sat, 6 Aug 2011 10:39:51 -0400
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	Joe Jin <joe.jin@...cle.com>
Cc:	Daniel Stodden <daniel.stodden@...rix.com>,
	Jens Axboe <jaxboe@...ionio.com>,
	Annie Li <annie.li@...cle.com>,
	Ian Campbell <Ian.Campbell@...citrix.com>,
	Kurt C Hackel <KURT.HACKEL@...cle.com>,
	Greg Marsden <greg.marsden@...cle.com>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen_blkif
 and some declares

On Thu, Aug 04, 2011 at 03:23:41PM +0800, Joe Jin wrote:
> 
> Add remove_requested to xen_blkif and some declares.

By itself this patch is a bit strange. If you look in
Documentation/SubmittingPatches:
"2) Describe your changes.

Describe the technical detail of the change(s) your patch includes.
"

There is no really technical details. As in, why is this patch neccessary.

That document also states:
"3) Separate your changes.

Separate _logical changes_ into a single patch file.

For example, if your changes include both bug fixes and performance
enhancements for a single driver, separate those changes into two
or more patches.  If your changes include an API update, and a new
driver which uses that new API, separate those into two patches."


This patch by itself has no logical purpose - as in it does not fix a bug,
or add a new feature - it just plops a new element in a structure, provides
two function decleration of non-existing functions and a maccro that is not
used.

Soo.  Looking at the three patches I believe some reshuffling ought to be done.
If you recall my comments:


        >  	case XenbusStateUnknown:
        > -		/* implies blkif_disconnect() via blkback_remove() */
        > +		/* implies xen_blkif_disconnect() via blkback_remove() */

        Ok, that is not part of this patch. You should create another commit which
        does this type of cleanup and
        >  		device_unregister(&dev->dev);
        >  		break;
        >  
        > @@ -620,6 +778,8 @@ static void frontend_changed(struct xenbus_device *dev,
        >  				 frontend_state);
        >  		break;
        >  	}
        > +
        > +	DPRINTK("%s: %s", dev->nodename, xenbus_strstate(dev->state));

        .. also for this.
        >  }
        >  

I am not sure if it is not clear, but what I meant that those two changes
(the comment rename and adding the DPRINKT) should be as a seperate
patch. So take those changes from patch #3 out and make a new patch.

Read on..
> 
> Signed-off-by: Joe Jin <joe.jin@...cle.com>
> Cc: Daniel Stodden <daniel.stodden@...rix.com>
> Cc: Jens Axboe <jaxboe@...ionio.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> Cc: Annie Li <annie.li@...cle.com>
> Cc: Ian Campbell <Ian.Campbell@...citrix.com>
> ---
>  drivers/block/xen-blkback/common.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index 9e40b28..acda757 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -49,6 +49,7 @@
>  	pr_debug(DRV_PFX "(%s:%d) " fmt ".\n",	\
>  		 __func__, __LINE__, ##args)
>  
> +#define WPRINTK(fmt, args...) printk(KERN_WARNING "xen-blkback: " fmt, ##args)

This is what I said in my review

        "What is 'WPRITNK' ? Can you use the the same type of printks
        as the rest? Also you have a space at the end. Make sure to
        run checkpath.pl"

which honeselty I should have been more specific. I meant that
you could just convert all the "WPRINTK" to what they define.

As in, sed/WPRINT/printk(KERN_WARNING/..

In essence, what I would like you to do is:

1). Read up on Documentation/SubmittingPatches
2). Squash this patch (except the declerations of the functions that are
    implemented in patch #3) into patch #2. The declerations of functions
    should be squashed in patch #3.
3). Replace in patch #3 all calls to WPRINTK with printk(KERN_WARNING.
4). Create a new patch that deals with the addition of DPRINTK
    and the update of the comment (see above).
5). The total should be three patches:
    a). This patch squashed with patch #2 (with the modification described in 2).
    b). Patch #3 modified
    c). A new patch with the debug/comments modifications.
6). Make sure each patch compiles on its own.
7). Resend - or if you want to double check with me in case I've further
    comments - just send it to me privately.

>  
>  /* Not a real protocol.  Used to generate ring structs which contain
>   * the elements common to all protocols only.  This way we get a
> @@ -145,6 +146,7 @@ struct xen_blkif {
>  	/* Back pointer to the backend_info. */
>  	struct backend_info	*be;
>  	/* Private fields. */
> +	bool			remove_requested;
>  	spinlock_t		blk_ring_lock;
>  	atomic_t		refcnt;
>  
> @@ -198,6 +200,9 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
>  
>  struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be);
>  
> +void xen_vbd_sync(struct xen_vbd *vbd);
> +void xen_blkback_close(struct xen_blkif *blkif);
> +
>  static inline void blkif_get_x86_32_req(struct blkif_request *dst,
>  					struct blkif_x86_32_request *src)
>  {
--
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/