[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] 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
"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

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 
        does this type of cleanup and
        >               device_unregister(&dev->dev);
        >               break;
        > @@ -620,6 +778,8 @@ static void frontend_changed(struct xenbus_device 
        >                                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 
    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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.