Philippe Mathieu-Daudé <f4...@amsat.org> writes:
> On 7/17/20 10:27 AM, Philippe Mathieu-Daudé wrote:
>> On 7/17/20 10:03 AM, Thomas Huth wrote:
>>> On 17/07/2020 09.48, Philippe Mathieu-Daudé wrote:
>>>> +Thomas
>>>
>>>> On 7/16/20 10:56 PM, Havard Skinnemoen wrote:
>>>>> On Wed, Jul 15, 2020 at 1:54 PM Havard Skinnemoen
>>>>> <hskinnem...@google.com> wrote:
>>>>>>
>>>>>> On Wed, Jul 15, 2020 at 3:57 AM Philippe Mathieu-Daudé <f4...@amsat.org>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 7/15/20 11:00 AM, Markus Armbruster wrote:
>>>>>>>> Now my point. Why first make up user configuration, then use that to
>>>>>>>> create a BlockBackend, when you could just go ahead and create the
>>>>>>>> BlockBackend?
>>>>>>>
>>>>>>> CLI issue mostly.
>>>>>>>
>>>>>>> We can solve it similarly to the recent "sdcard: Do not allow invalid SD
>>>>>>> card sizes" patch:
>>>>>>>
>>>>>>> if (!dinfo) {
>>>>>>> error_setg(errp, "Missing SPI flash drive");
>>>>>>> error_append_hint(errp, "You can use a dummy drive using:\n");
>>>>>>> error_append_hint(errp, "-drive if=mtd,driver=null-co,"
>>>>>>> "read-ones=on,size=64M\n);
>>>>>>> return;
>>>>>>> }
>>>>>>>
>>>>>>> having npcm7xx_connect_flash() taking an Error* argument,
>>>>>>> and MachineClass::init() call it with &error_fatal.
>>>>>>
>>>>>> Erroring out if the user specifies a configuration that can't possibly
>>>>>> boot sounds good to me. Better than trying to come up with defaults
>>>>>> that are still not going to result in a bootable system.
>>>>>>
>>>>>> For testing recovery paths, I think it makes sense to explicitly
>>>>>> specify a null device as you suggest.
>>>>>
>>>>> Hmm, one problem. qom-test fails with
>>>>>
>>>>> qemu-system-aarch64: Missing SPI flash drive
>>>>> You can add a dummy drive using:
>>>>> -drive if=mtd,driver=null-co,read-zeroes=on,size=32M
>>>>> Broken pipe
>>>>> /usr/local/google/home/hskinnemoen/qemu/for-upstream/tests/qtest/libqtest.c:166:
>>>>> kill_qemu() tried to terminate QEMU process but encountered exit
>>>>> status 1 (expected 0)
>>>>> ERROR qom-test - too few tests run (expected 68, got 7)
>>>>>
>>>>> So it looks like we might need a different solution to this, unless we
>>>>> want to make generic tests more machine-aware...
>>>
>>> I didn't follow the other mails in this thread, but what we usually do
>>> in such a case: Add a "if (qtest_enabled())" check to the device or the
>>> machine to ignore the error if it is running in qtest mode.
>>
>> Hmm I'm not sure it works in this case. We could do:
>>
>> if (!dinfo) {
>> if (qtest) {
>> /* create null drive for qtest */
>> opts = ...;
>> dinfo = drive_new(opts, IF_MTD, &error_abort);
>> } else {
>> /* teach user to use proper CLI */
>> error_setg(errp, "Missing SPI flash drive");
>> error_append_hint(errp, "You can use a dummy drive using:\n");
>> error_append_hint(errp, "-drive if=mtd,driver=null-co,"
>> "read-ones=on,size=64M\n);
>> }
>> }
>>
>> But I'm not sure Markus will enjoy it :)
Using drive_new() for creating an internal dummy backend is wrong.
Doing it only when qtest_enabled() doesn't make it less wrong.
>> Markus, any better idea about how to handle that with automatic qtests?
>
> FWIW IDE device has a concept of "Anonymous BlockBackend for an empty
> drive":
>
> static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
> {
> IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
> IDEState *s = bus->ifs + dev->unit;
> int ret;
>
> if (!dev->conf.blk) {
> if (kind != IDE_CD) {
> error_setg(errp, "No drive specified");
> return;
> } else {
> /* Anonymous BlockBackend for an empty drive */
> dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
> ret = blk_attach_dev(dev->conf.blk, &dev->qdev);
> assert(ret == 0);
> }
> }
I figure this creates an internal dummy backend the right way, just not
the kind you need. For a non-empty one, you get to make up a
BlockDriverState, then use blk_new_with_bs().
Is the simplification of device code really worth making up a dummy
backend?
