On 17.09.2021 15:26, Andrew Cooper wrote: > On 17/09/2021 13:58, Jan Beulich wrote: >> On 17.09.2021 10:45, Andrew Cooper wrote: >>> --- a/xen/common/trace.c >>> +++ b/xen/common/trace.c >>> @@ -686,22 +686,21 @@ void __trace_var(u32 event, bool_t cycles, unsigned >>> int extra, >>> unsigned long flags; >>> u32 bytes_to_tail, bytes_to_wrap; >>> unsigned int rec_size, total_size; >>> - unsigned int extra_word; >>> bool_t started_below_highwater; >>> >>> if( !tb_init_done ) >>> return; >>> >>> - /* Convert byte count into word count, rounding up */ >>> - extra_word = (extra / sizeof(u32)); >>> - if ( (extra % sizeof(u32)) != 0 ) >>> - extra_word++; >>> - >>> - ASSERT(extra_word <= TRACE_EXTRA_MAX); >>> - extra_word = min_t(int, extra_word, TRACE_EXTRA_MAX); >>> - >>> - /* Round size up to nearest word */ >>> - extra = extra_word * sizeof(u32); >>> + /* >>> + * Trace records require extra data which is an exact multiple of >>> + * uint32_t. Reject out-of-spec records. Any failure here is an >>> error in >>> + * the caller. >>> + */ >> Hmm, is "require" accurate? > > In terms of "what will go wrong if this condition is violated", yes. > >> They may very well come without extra data >> afaics. > > 0 is fine, and used by plenty of records, and also permitted by the > filtering logic.
I was about to say that the two parts of your reply contradict one another, when I finally realized that it looks like the first sentence in the comment can be read two ways: "Trace records require extra data" then going on to describe properties, or "Trace records require extra data to be an exact multiple of uint32_t." Obviously this is to me as a non-native speaker. But maybe you could still reword this to be unambiguous? (I'm not going to exclude that the lack of a comma, which I did silently add while reading, makes a difference here: Does "Trace records require extra data, which is an exact multiple of uint32_t" end up altering the meaning?) Jan
