On 24/10/2023 14:29, David Woodhouse wrote:
On Tue, 2023-10-24 at 13:59 +0100, Paul Durrant wrote:
On 24/10/2023 13:56, David Woodhouse wrote:
On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
{
ERRP_GUARD();
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+ XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
- xendev->frontend_path = xen_device_get_frontend_path(xendev);
+ if (xendev_class->get_frontend_path) {
+ xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+ if (!xendev->frontend_path) {
+ return;
I think you need to update errp here to note that you are failing to
create the frontend.
If xendev_class->get_frontend_path returned NULL it will have filled in errp.
Ok, but a prepend to say that a lack of path there means we skip
frontend creation seems reasonable?
No, it *is* returning an error. Perhaps I can make it
I understand it is returning an error. I thought the point of the
cascading error handling was to prepend text at each (meaningful) layer
such that the eventual message conveyed what failed and also what the
consequences of that failure were.
Paul
if (!xendev->frontend_path) {
/*
* If the ->get_frontend_path() method returned NULL, it will
* already have set *errp accordingly. Return the error.
*/
return /* false */;
}
As a general rule (I'll be doing a bombing run on xen-bus once I get my
patch queue down into single digits) we should never check 'if (*errp)'
to check if a function had an error. It should *also* return a success
or failure indication, and we should cope with errp being NULL.
I'm pretty sure someone told me the exact opposite a few years back.
Then they were wrong :)
