Re: [PULL 37/40] monitor: Tidy up find_device_state()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Christian Borntraeger <borntraeger@de.ibm.com> writes:
>
>> Am 13.10.21 um 11:07 schrieb Paolo Bonzini:
>>> From: Markus Armbruster <armbru@redhat.com>
>>> Commit 6287d827d4 "monitor: allow device_del to accept QOM paths"
>>> extended find_device_state() to accept QOM paths in addition to qdev
>>> IDs.  This added a checked conversion to TYPE_DEVICE at the end, which
>>> duplicates the check done for the qdev ID case earlier, except it sets
>>> a *different* error: GenericError "ID is not a hotpluggable device"
>>> when passed a QOM path, and DeviceNotFound "Device 'ID' not found"
>>> when passed a qdev ID.  Fortunately, the latter won't happen as long
>>> as we add only devices to /machine/peripheral/.
>>> Earlier, commit b6cc36abb2 "qdev: device_del: Search for to be
>>> unplugged device in 'peripheral' container" rewrote the lookup by qdev
>>> ID to use QOM instead of qdev_find_recursive(), so it can handle
>>> buss-less devices.  It does so by constructing an absolute QOM path.
>>> Works, but object_resolve_path_component() is easier.  Switching to it
>>> also gets rid of the unclean duplication described above.
>>> While there, avoid converting to TYPE_DEVICE twice, first to check
>>> whether it's possible, and then for real.
>>
>> This one broke qemu iotest 280 on s390:
>>
>>
>> 280   fail       [13:06:19] [13:06:19]   0.3s   (last: 0.3s)  output 
>> mismatch (see 280.out.bad)
>> --- /home/cborntra/REPOS/qemu/tests/qemu-iotests/280.out
>> +++ 280.out.bad
>> @@ -37,14 +37,14 @@
>>  === Resume the VM and simulate a write request ===
>>  {"execute": "cont", "arguments": {}}
>>  {"return": {}}
>> -{"return": ""}
>> +{"return": "Error: Device 'vda/virtio-backend' not found\r\n"}
>>
>>  === Commit it to the backing file ===
>>  {"execute": "block-commit", "arguments": {"auto-dismiss": false, "device": 
>> "top-fmt", "job-id": "job0", "top-node": "top-fmt"}}
>>  {"return": {}}
>>  {"execute": "job-complete", "arguments": {"id": "job0"}}
>>  {"return": {}}
>> -{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, 
>> "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
>> "USECS", "seconds": "SECS"}}
>> -{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, 
>> "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
>> {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": 
>> "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
>> "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": 
>> "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": 
>> "USECS", "seconds": "SECS"}}
>>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>>  {"return": {}}
>> Failures: 280
>> Failed 1 of 1 iotests
>
> Reproduced.  I'll dig deeper.  Thanks!

Classical case of failing to adhere to "read only the code": reading the
documentation lodged "this is a qdev ID" in my brain, blinding me to the
fact that the code actually treats it as a QOM path relative to
/machine/peripheral/.

Sorry!

Please try "[PATCH] monitor: Fix find_device_state() for IDs containing
slashes".