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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.05.17 at 14:42, <ian.jackson@xxxxxxxxxxxxx> wrote:
> 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'

I admit that in the given case I didn't go look in what state the file
as a whole is. Yet while indeed I _could_ do better with my
comment, I even more so think that submitters _could_ do better
by not introducing obvious style violations. The more I as a
reviewer see such, the more I'm inclined to use very terse
comments. I realize this may occasionally be unfair, as the
particular contributor may not have had much other history. But
please do realize that with the amount of patches these days
being _far_ beyond what I can reasonably review (and apparently
others too, or I'd have seen quite a few more reviews recently), it
doesn't look very reasonable for me to always go and write
extended explanations of what's wrong (and no, I don't fancy
simply skipping such cosmetic items - when they're really easy to
correct I sometimes offer fixing them while committing). I try to do
so when I recognize newcomers.

> 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.

And indeed I've given the comment because of both of the issues
you name.

> Thanks, and sorry to tetch.

While I can vaguely guess what you mean here, would you mind
helping out with neither me nor my dictionary knowing the word
"tetch"?

Jan


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