Re: [Xen-devel] [PATCH] XenBus: Don't wait for producer to fill the ring if the ring

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Jan Beulich writes ("Re: [Xen-devel] [PATCH] XenBus: Don't wait for producer to 
fill the ring if the ring"):
> On 17.05.17 at 16:57, <anshul.makkar@xxxxxxxxxx> wrote:
> > --- a/tools/firmware/hvmloader/xenbus.c
> > +++ b/tools/firmware/hvmloader/xenbus.c
> > @@ -141,7 +141,18 @@ static void ring_read(char *data, uint32_t len)
> >          /* Don't overrun the producer pointer */
> >          while ( (part = MASK_XENSTORE_IDX(rings->rsp_prod -
> >                                            rings->rsp_cons)) == 0 )
> > +        {
> > +            /* don't wait for producer to fill the ring if it is already 
> > full.
> > +             * Condition happens when you write string > 1K into the ring.
> > +             * eg case prod=1272 cons=248.
> > +             */
> 
> Comment style.

What specifically are you complaining about here ?

If you're talking about the fact that the opening /* is on the same
line as the first line of text, observe that the rest of
hvmloader/xenbus.c has most multi-line comments in these styles:

 /* If the backend reads the state while we're erasing it then the
  * ring state will become corrupted, preventing guest frontends from
  * connecting. This is rare. To help diagnose the failure, we fill
  * the ring with XS_INVALID packets. */

 /* Read a xenstore key.  Returns a nul-terminated string (even if the XS
  * data wasn't nul-terminated) or NULL.  The returned string is in a
  * static buffer, so only valid until the next xenstore/xenbus operation.
  * If @default_resp is specified, it is returned in preference to a NULL or
  * empty string received from xenstore.
  */

The only exceptions are the copyright notice and the local variables
block.

So if this is what you are complaining about, I think this is
unhelpful nit-picking, given the state of rest of the hvmloader source
code.  It obviously doesn't really matter whether /* and */ are on a
line by themselves.

At the very least, if you're going to insist on some particular style,
you could:

 * acknowledge that the existing style is not always consistent
   and that therefore the submitter couldn't necessarily be expected
   to get this "right" (whatever that means) without help

 * clearly state what your problem is and which style is to be
   used (instead of the obvious candidate which is the style
   of the surrounding code) rather than just writing `comment style'


Or maybe you are talking about the lack of a capital d in "don't" ?
I agree that a capital letter would be better.  But I think "comment
style" is quite an opaque way of making that observation.

Or perhaps you are talking about some other perceived problem with
this, but of course the reader of your message needs to try to guess
from what you wrote.

Thanks, and sorry to tetch.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel