| Message ID | 20231016151909.22133-7-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Get Xen PV shim running in qemu | expand |
On 16/10/2023 16:19, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > The primary Xen console is special. The guest's side is set up for it by > the toolstack automatically and not by the standard PV init sequence. > > Accordingly, its *frontend* doesn't appear in …/device/console/0 either; > instead it appears under …/console in the guest's XenStore node. > > To allow the Xen console driver to override the frontend path for the > primary console, add a method to the XenDeviceClass which can be used > instead of the standard xen_device_get_frontend_path() > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/xen/xen-bus.c | 10 +++++++++- > include/hw/xen/xen-bus.h | 2 ++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > index ece8ec40cd..cc524ed92c 100644 > --- 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. Paul
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. 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.
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? > 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. Paul
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 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 :)
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 :)
On Tue, 2023-10-24 at 14:37 +0100, Paul Durrant wrote: > > > > 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. Oh, I see. Added; thanks.
On Tue, 2023-10-24 at 14:29 +0100, David Woodhouse wrote: > > > > 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 :) cf. https://gitlab.com/qemu-project/qemu/-/commit/e3fe3988d7 "Whenever practical, also return a value that indicates success / failure"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index ece8ec40cd..cc524ed92c 100644 --- 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; + } + } else { + xendev->frontend_path = xen_device_get_frontend_path(xendev); + } /* * The frontend area may have already been created by a legacy diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h index f435898164..eb440880b5 100644 --- a/include/hw/xen/xen-bus.h +++ b/include/hw/xen/xen-bus.h @@ -33,6 +33,7 @@ struct XenDevice { }; typedef struct XenDevice XenDevice; +typedef char *(*XenDeviceGetFrontendPath)(XenDevice *xendev, Error **errp); typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp); typedef void (*XenDeviceRealize)(XenDevice *xendev, Error **errp); typedef void (*XenDeviceFrontendChanged)(XenDevice *xendev, @@ -46,6 +47,7 @@ struct XenDeviceClass { /*< public >*/ const char *backend; const char *device; + XenDeviceGetFrontendPath get_frontend_path; XenDeviceGetName get_name; XenDeviceRealize realize; XenDeviceFrontendChanged frontend_changed;