Re: [Qemu-devel] [PATCH qemu] loader: Trace loaded images

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/17/19 3:25 AM, Alexey Kardashevskiy wrote:
> On 14/06/2019 19:33, Stefan Hajnoczi wrote:
>> On Fri, Jun 14, 2019 at 10:13:04AM +1000, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 13/06/2019 23:08, Philippe Mathieu-Daudé wrote:
>>>> Hi Alexey,
>>>>
>>>> On 6/13/19 7:09 AM, Alexey Kardashevskiy wrote:
>>>>> This adds a trace point which prints every loaded image. This includes
>>>>> bios/firmware/kernel/initradmdisk/pcirom.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>>
>>>>> The example for a pseries guest:
>>>>>
>>>>> loader_write_rom slof.bin: @0x0 size=0xe22e0 ROM=0
>>>>> loader_write_rom phdr #0: /home/aik/t/vml4120le: @0x400000 size=0x13df000 ROM=0
>>>>> loader_write_rom /home/aik/t/le.cpio: @0x1ad0000 size=0x9463a00 ROM=0
>>>>
>>>> I find the "ROM=0" part confuse, maybe you can change to "ROM:false".
>>>
>>> How? I mean I can do that in the code as rom->isrom?"true":"false" and
>>> make trace point accept "%s" but it is quite ugly and others seem to
>>> just use %d for bool.
>>
>> Yes, %d is the convention for bool.  Perhaps you can name it "is_rom"
>> instead of "ROM".  That way the name communicates that this is a boolean
>> value.
> 
> It is quite obvious though that it is boolean even as "ROM" (what else
> can that be realistically?) and there does not seem to be a convention
> about xxx:N vs is_xxx:N. And personally I find longer lines worse for
> limited width screens (I run multiple qemus in tiled tmux). Whose tree
> is this going to? Let's ask that person :)

Personally I find 'is_rom' clearer. I read 2 addresses, then my first
reaction was to parse it as another address. But it is also true we now
enforce traced hex values with '0x' prefix, so your 'ROM' is unlikely an
address. Tiled tmux is an acceptable argument. Anyway you already got my
R-b.

Tree: the PPC tree is likely to get it merged quicker than the MISC tree.

Regards,

Phil.