On 6/8/20 7:20 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <phi...@redhat.com> writes: > >> On 6/5/20 4:56 PM, Markus Armbruster wrote: >>> qdev_prop_set_drive() can fail. None of the other qdev_prop_set_FOO() >>> can; they abort on error. >>> >>> To clean up this inconsistency, rename qdev_prop_set_drive() to >>> qdev_prop_set_drive_err(), and create a qdev_prop_set_drive() that >>> aborts on error. >>> >>> Coccinelle script to update callers: >>> >>> @ depends on !(file in "hw/core/qdev-properties-system.c")@ >>> expression dev, name, value; >>> symbol error_abort; >>> @@ >>> - qdev_prop_set_drive(dev, name, value, &error_abort); >>> + qdev_prop_set_drive(dev, name, value); >> >> Why not open-code qdev_prop_set_drive_err(..., &error_abort)? > > Consistency with qdev_prop_set_chr() and qdev_prop_set_netdev(). > > My starting point was "what makes block backends so different that they > need error handling where nothing else does?" > > After a considerable amount of digging, my answer is "nothing". > qdev_prop_set_drive(), qdev_prop_set_chr() and qdev_prop_set_netdev() > can all run into errors. On closer examination, all programming errors > (thus &error_abort), except for "backend is already in use", and to > trigger that one, you have to get creative and steal the backend for > another purpose, e.g. with -global. This is the abridged version of a > longwinded argument I didn't want to make in this series, so I left the > error handling alone. > > In the longer run, I want qdev_prop_set_drive_err() to die.
I agree with the longer run. I naively thought this could be done in the same patch. Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> > >> >>> >>> @@ >>> expression dev, name, value, errp; >>> @@ >>> - qdev_prop_set_drive(dev, name, value, errp); >>> + qdev_prop_set_drive_err(dev, name, value, errp); >>> >> [...] >