Re: [Qemu-devel] Patch to add helpful tracing output for driver authors in NVMe emulation

[Doug Gale]

On Tue, Oct 10, 2017 at 4:02 AM, Kevin Wolf <address@hidden> wrote:
> Am 10.10.2017 um 08:58 hat Markus Armbruster geschrieben:
>> Doug Gale <address@hidden> writes:
>>
>> > I used exclamations as a concise way of indicating that the driver did
>> > something nonsensical, or horribly invalid, like something likely to
>> > cause a memory corruption, trying to start the controller with a
>> > nonsense configuration, providing invalid PRDs or writing to
>> > unrecognized MMIO offsets that might hang or do something extremely
>> > bad on a poorly implemented controller. Exclaiming is not shouting, I
>> > thought shouting was when it was all uppercase.
>> >
>> > If a driver might trash memory or hang a controller, maybe it should
>> > shout at the driver author or person investigating an unstable VM.
>> > None of those messages with exclamations should ever happen. If any of
>> > them are possible when the driver is correct, then I have made a
>> > mistake.
>>
>> Please do not top-quote on this mailing list.
>>
>> Existing tracepoints do not use exclamation marks to signal severity.
>>
>> Consider using assertions for "if this happens, either the program's
>> state is shot (and continuing is unsafe), or the author's mental state
>> was shot (and continuing is probably unsafe, too)" conditions.
>> Tracepoints are for tracing.
>
> Assertions are for checking that assumptions in qemu code hold true.
> Here it's about bad guest code, and you can't let qemu abort for that.
>
> Tracing is the right tool to detect bad guest code, and I think it makes
> sense to mark conditions that shouldn't happen with a correctly
> operating guest driver. I'm not sure if an exclamation mark is the best
> syntax for this, because I wouldn't have intuitively understood what
> it's supposed to tell me.
>
> If we do use the exclamation mark, we should document it somewhere
> (docs/devel/tracing.txt?) and make it a qemu-wide pattern. If not, we
> should choose something else and still document it and use it
> consistently.
>
> Kevin
>

Should I give up on indicating severity for now (removing the
exclamations) and just group together the severe ones with a comment
heading in the trace-events file?
I added "" at the end of some traces to help possibly-incorrect
parsing code find the end of the string reliably. Was that a good idea
or is it okay and expected to end them with something like PRIx64 and
drop the extra ""?
I found a few cases of missing 0x that I will fix in the next version
of this patch. Policy is to have 0x before every hex value, right?

Also, so I'm clear, when I submit the patch I should put the patch
first and put my message after the -- at the end?
And use "nvme: Add tracing" for the commit message,
And put nvme: Add tracing v3 in my subject line in a completely new
email thread?