[Xen-devel] Re: [PATCH -v3 1/3] xen-blkback: add remove_requested to xen_blkif and some declares

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Cc: Daniel Stodden <daniel.stodden@xxxxxxxxxx>
> Cc: Jens Axboe <jaxboe@xxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Annie Li <annie.li@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> ---
>  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)
>  {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel