Re: [Qemu-devel] [PATCH v2 1/1] block: improve error handling in raw_open

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3594 bytes --]

On 27.07.2016 14:40, Halil Pasic wrote:
> 
> 
> On 07/26/2016 07:47 PM, Max Reitz wrote:
>>> Frankly, I'm a bit uncomfortable with asking (do not want to be pushy),
>>>> but do you have an opinion on the 'error_report_err' issue (pointed
>>>> out in the cover letter part)?
>> You are using drive_add with QMP? As far as I know, one can only do so
>> with human-monitor-command which returns the error string like so:
>>
>> {"return": "Could not open file: No such file or directory\r\n"}
>>
> 
> Yes, libvirt uses human-monitor-command, but are you sure the error
> message is propagated back as described above?
> 
> Here is the call chain I'm talking about:
> 
> #0  error_report_err (err=0x1329a9d0) at /mnt/devel/root/git/qemu/util/error.c:228
> #1  0x00000000100de4fe in drive_new (all_opts=all_opts@entry=0x12044010, block_default_type=<optimized out>) at /mnt/devel/root/git/qemu/blockdev.c:1134
> #2  0x00000000100e47be in add_init_drive (optstr=<optimized out>) at /mnt/devel/root/git/qemu/device-hotplug.c:46
> #3  hmp_drive_add (mon=0x3ffe3f7d188, qdict=0x120b3af0) at /mnt/devel/root/git/qemu/device-hotplug.c:66
> #4  0x0000000010052092 in handle_hmp_command (mon=mon@entry=0x3ffe3f7d188, cmdline=0x110399ba "dummy file=/dev/sg924,if=none,id=drive-hostdev912", cmdline@entry=0x110399b0 "drive_add dummy file=/dev/sg924,if=none,id=drive-hostdev912")
>     at /mnt/devel/root/git/qemu/monitor.c:2929
> #5  0x0000000010052176 in qmp_human_monitor_command (command_line=0x110399b0 "drive_add dummy file=/dev/sg924,if=none,id=drive-hostdev912", has_cpu_index=<optimized out>, cpu_index=0, errp=errp@entry=0x3ffe3f7d310)
>     at /mnt/devel/root/git/qemu/monitor.c:668
> #6  0x00000000100f6faa in qmp_marshal_human_monitor_command (args=<optimized out>, ret=0x3ffe3f7d400, errp=0x3ffe3f7d3f8) at qmp-marshal.c:1777
> [...]
> 
> Now if you examine #1 drive_new(all_opts,block_default_type) you see,
> there is no errp argument and if you examine the code you see that the
> error from blockdev_init which gets propagated properly to this point
> gets "handled" by error_report_err (in QMP context! so does not much
> good on this code path). AFAIU this can not work. Or am I wrong?

drive_add is an HMP command. There's no other way for it to emit errors.

Strictly speaking, HMP commands are not supposed to be used by
management applications like libvirt (the "H" stands for "human", after
all). QMP's "human-monitor-command" is just a workaround because there
are some HMP commands for which we do not have fully working QMP
replacements yet. One such example is indeed drive_add, because as
Markus correctly pointed out, blockdev-add is still considered experimental.

So we're still in the awkward spot of only having a legacy command
(drive_add) and an experimental command (blockdev-add); and we have been
in that spot for quite a while now (more than two years, I think). I
think we're getting rather close to getting blockdev-add stable, but
then again I'm afraid that might be something we've been thinking for
the past two years.

Max

>> Apart from that, as for QMP, in theory you're supposed to use
>> blockdev-add and device_add, I think (although blockdev-add is still
>> considered experimental...). And blockdev-add will return the error
>> string like so:
>>
>> {"error": {"class": "GenericError", "desc": "Could not open file: No
>> such file or directory"}}
>>
> 
> I guess libvirt will eventually pick up blockdev-add so the problem
> should eventually go away.
> 
> Cheers,
> Halil
> 
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]