Re: [win-pv-devel] [PATCH] V2

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Paul Durrant
> Sent: 16 July 2019 09:10
> To: Daniel Davis <daniel.davis@xxxxxxxxxx>; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Daniel Davis <daniel.davis@xxxxxxxxxx>
> Subject: Re: [win-pv-devel] [PATCH] V2
> 
> > -----Original Message-----
> > From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> > Daniel Davis
> > Sent: 15 July 2019 16:38
> > To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Daniel Davis <daniel.davis@xxxxxxxxxx>
> > Subject: [win-pv-devel] [PATCH] V2
> >
> > Signed-off-by: Daniel Davis <Daniel.Davis@xxxxxxxxxx>
> 
> Sorry if I was not clear. What you should do in response to review comments 
> is send a replacement
> patch (not an incremental one) with the same subject as the prior patch but 
> with an extra 'v2'. So, in
> your case, since you sent a series of 2. You need to re-send the whole 
> series, so your email subjects
> would be:
> 
> [PATCH v2 1/2] rate limit BLKIF_OP failure log messages XSI-413

Actually I had not noticed that you refer to a Citrix internal ticket number 
here. Please don't do that as they are meaningless to the community in general.

  Paul

> [PATCH v2 2/2] rate limited log messages for storage errors
> 
> Although it appears you can drop patch #2 since it appears to be fixing the 
> lack of 'ull' suffix I
> pointed out in my review of patch #1.
> 
> If you run 'git format-patch --subject-prefix="PATCH v2" -2 HEAD' against 
> your branch (assuming your
> latter patch is HEAD) then you will generate patches with the appropriate 
> subject lines. However, now
> patch #2 is moot you should be able to get rid of it and then you send run 
> 'git send-email --subject-
> prefix="PATCH v2" -1 HEAD' to generate and send the patch in one go.
> 
> For general guidance have a read of 
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches as
> this is largely the process we use for the PV drivers. It's also a good idea 
> to look back down the
> mailing list archives 
> (https://lists.xenproject.org/archives/html/win-pv-devel/) to get a feel for 
> how
> things are done.
> 
> > ---
> >  src/xenvbd/ring.c | 35 +++++++++++++++++------------------
> >  1 file changed, 17 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/xenvbd/ring.c b/src/xenvbd/ring.c
> > index 431e1b5..b497690 100644
> > --- a/src/xenvbd/ring.c
> > +++ b/src/xenvbd/ring.c
> > @@ -95,7 +95,7 @@ typedef struct _XENVBD_BLKIF_RING {
> >      ULONG                           RequestsPushed;
> >      ULONG                           ResponsesProcessed;
> >      PXENBUS_DEBUG_CALLBACK          DebugCallback;
> > -   LARGE_INTEGER                                   TimeOfLastErrorLog;
> > +    LARGE_INTEGER                   TimeOfLastErrorLog;
> >  } XENVBD_BLKIF_RING, *PXENVBD_BLKIF_RING;
> >
> >  typedef enum _XENVBD_STAT {
> > @@ -1214,23 +1214,22 @@ __BlkifRingCompleteResponse(
> >          break;
> >
> >      case BLKIF_RSP_ERROR:
> > -    default:
> > -   {
> > -           LARGE_INTEGER timeNow;
> > -
> > -           KeQuerySystemTime(&timeNow);
> > -
> > -           //if last log message was less than 10 seconds ago
> > -           if (timeNow.QuadPart - BlkifRing->TimeOfLastErrorLog.QuadPart < 
> > 100000000) {
> > -                   Warning("Target[%u][%u] : %s BLKIF_RSP_ERROR\n",
> > -                           FrontendGetTargetId(Frontend),
> > -                           BlkifRing->Index,
> > -                           __BlkifOperationName(Request->Operation));
> > -                   KeQuerySystemTime(&BlkifRing->TimeOfLastErrorLog);
> > -           }
> > -           Srb->SrbStatus = SRB_STATUS_ERROR;
> > -           break;
> > -           }
> > +    default:{
> 
> You need a space between the ':' and the brace.
> 
> > +        LARGE_INTEGER TimeNow;
> > +
> > +        KeQuerySystemTime(&TimeNow);
> > +
> > +        //if last log message was less than 10 seconds ago
> 
> Space after '//' and capital letter to start the comment please.
> 
> > +        if (TimeNow.QuadPart - BlkifRing->TimeOfLastErrorLog.QuadPart < 
> > 100000000ull) {
> > +            Warning("Target[%u][%u] : %s BLKIF_RSP_ERROR\n",
> > +            FrontendGetTargetId(Frontend),
> > +            BlkifRing->Index,
> > +            __BlkifOperationName(Request->Operation));
> 
> Your indentation seems to have gone wrong for the above three lines. They 
> should be aligned with the
> first parameter to Warning().
> 
>   Paul
> 
> > +            KeQuerySystemTime(&BlkifRing->TimeOfLastErrorLog);
> > +        }
> > +        Srb->SrbStatus = SRB_STATUS_ERROR;
> > +        break;
> > +      }
> >      }
> >
> >      BlkifRingPutRequest(BlkifRing, Request);
> > --
> > 2.22.0.windows.1
> >
> >
> > _______________________________________________
> > win-pv-devel mailing list
> > win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/win-pv-devel
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/win-pv-devel