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;
+        }
+    } 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;
-- 
2.40.1

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 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"

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.