Re: [Xen-devel] [PATCH v2 02/13] xen/pvcalls: connect to the backend

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

>>>   static int pvcalls_front_probe(struct xenbus_device *dev,
>>>                       const struct xenbus_device_id *id)
>>>   {
>>> +   int ret = -EFAULT, evtchn, ref = -1, i;
>>> +   unsigned int max_page_order, function_calls, len;
>>> +   char *versions;
>>> +   grant_ref_t gref_head = 0;
>>> +   struct xenbus_transaction xbt;
>>> +   struct pvcalls_bedata *bedata = NULL;
>>> +   struct xen_pvcalls_sring *sring;
>>> +
>>> +   if (pvcalls_front_dev != NULL) {
>>> +           dev_err(&dev->dev, "only one PV Calls connection
>>> supported\n");
>>> +           return -EINVAL;
>>> +   }
>>> +
>>> +   versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
>>> +   if (!len)
>>> +           return -EINVAL;
>>> +   if (strcmp(versions, "1")) {
>>> +           kfree(versions);
>>> +           return -EINVAL;
>>> +   }
>>> +   kfree(versions);
>>> +   ret = xenbus_scanf(XBT_NIL, dev->otherend,
>>> +                      "max-page-order", "%u", &max_page_order);
>>> +   if (ret <= 0)
>>> +           return -ENODEV;
>>> +   if (max_page_order < RING_ORDER)
>>> +           return -ENODEV;
>>> +   ret = xenbus_scanf(XBT_NIL, dev->otherend,
>>> +                      "function-calls", "%u", &function_calls);
>>> +   if (ret <= 0 || function_calls != 1)
>>> +           return -ENODEV;
>>> +   pr_info("%s max-page-order is %u\n", __func__, max_page_order);
>>> +
>>> +   bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
>>> +   if (!bedata)
>>> +           return -ENOMEM;
>>> +
>>> +   init_waitqueue_head(&bedata->inflight_req);
>>> +   for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
>>> +           bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
>>> +
>>> +   sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
>>> +                                                        __GFP_ZERO);
>>> +   if (!sring)
>>> +           goto error;
>>> +   SHARED_RING_INIT(sring);
>>> +   FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
>>> +
>>> +   ret = xenbus_alloc_evtchn(dev, &evtchn);
>>> +   if (ret)
>>> +           goto error;
>>> +
>>> +   bedata->irq = bind_evtchn_to_irqhandler(evtchn,
>>> +                                           pvcalls_front_event_handler,
>>> +                                           0, "pvcalls-frontend", dev);
>>> +   if (bedata->irq < 0) {
>>> +           ret = bedata->irq;
>>> +           goto error;
>>> +   }
>>> +
>>> +   ret = gnttab_alloc_grant_references(1, &gref_head);
>>> +   if (ret < 0)
>>> +           goto error;
>>> +   bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);
>> Is ref really needed?
> No, I'll remove it
>
>
>>> +   if (ref < 0)
>>> +           goto error;
>>> +   gnttab_grant_foreign_access_ref(ref, dev->otherend_id,
>>> +                                   virt_to_gfn((void *)sring), 0);
>>> +
>>> + again:
>>> +   ret = xenbus_transaction_start(&xbt);
>>> +   if (ret) {
>>> +           xenbus_dev_fatal(dev, ret, "starting transaction");
>>> +           goto error;
>>> +   }
>>> +   ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
>>> +   if (ret)
>>> +           goto error_xenbus;
>>> +   ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%d", ref);
>>> +   if (ret)
>>> +           goto error_xenbus;
>>> +   ret = xenbus_printf(xbt, dev->nodename, "port", "%u",
>>> +                       evtchn);
>>> +   if (ret)
>>> +           goto error_xenbus;
>>> +   ret = xenbus_transaction_end(xbt, 0);
>>> +   if (ret) {
>>> +           if (ret == -EAGAIN)
>>> +                   goto again;
>>> +           xenbus_dev_fatal(dev, ret, "completing transaction");
>>> +           goto error;
>>> +   }
>>> +
>>> +   INIT_LIST_HEAD(&bedata->socket_mappings);
>>> +   INIT_LIST_HEAD(&bedata->socketpass_mappings);
>>> +   spin_lock_init(&bedata->pvcallss_lock);
>>> +   dev_set_drvdata(&dev->dev, bedata);
>>> +   pvcalls_front_dev = dev;
>>> +   xenbus_switch_state(dev, XenbusStateInitialised);
>>> +
>>>     return 0;
>>> +
>>> + error_xenbus:
>>> +   xenbus_transaction_end(xbt, 1);
>>> +   xenbus_dev_fatal(dev, ret, "writing xenstore");
>>> + error:
>>> +   pvcalls_front_remove(dev);
>> I think patch 12 (where you implement cleanup) could be moved before this 
>> one.
> I'll move the patch
>
>
>> I also think you are leaking bedata on error paths.
> bedata is freed by pvcalls_front_remove (kfree(bedata)), why do you say
> so?

bedata there is read from dev_get_drvdata() and here you assign drvdata
at the very end.

Come think of it, pvcalls_front_remove() should probably first check
whether bedata is valid. Or drvdata should be assigned right away in
this routine, before any 'got error/error_xenbus'.

-boris

>
>
>>> +   return ret;
>>>   }
>>>     


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel