Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 :)




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux