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

Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys


  • To: Jason Andryuk <jandryuk@xxxxxxxxx>
  • From: Ian Jackson <ian.jackson@xxxxxxxxxx>
  • Date: Mon, 18 May 2020 15:19:58 +0100
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Mon, 18 May 2020 14:20:11 +0000
  • Ironport-sdr: 5eQFsejmTheHw2nmIstzN+eVgeD8oLJB4A1YgKCxeb9/EOb7rzWB1NPVo1KCOhZj6wU0VObIbs x/pMBJF3seHorm5w1pSw4/HXuIXdCijeUdpdRsLl9UiiyiKq19pSKa0FXQ9GUDa6p23GWqp9Wy HysyP11ElT43tN0eOBFYdq5LkX9LX5Gq9jPWhLSlnqYxYE53naUUGgnMi+pFj6mrEYawwinjn4 +2kv5cOOtzOYPxXoxkKD8iZul/p/x17Af91A82HwRHlS9WXPIC/BUi1ktul5rF/vohrf/youfd gfA=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Jason Andryuk writes ("[PATCH v6 06/18] libxl: write qemu arguments into 
separate xenstore keys"):
> From: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
...
> +static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
> +                                           int dm_domid, int guest_domid,
> +                                           char **args)
> +{

Thanks for the changes.

> +    xs_transaction_t t = XBT_NULL;
...
> +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> +                                  GCSPRINTF("/local/domain/%d/vm", 
> guest_domid),
> +                                  &vm_path);
> +    if (rc)
> +        return rc;

I think this should be "goto out".  That conforms to standard libxl
error handling discipline and avoids future leak bugs etc.
libxl__xs_transaction_abort is a no-op with t==NULL.

Also, it is not clear to me why you chose to put this outside the
transaction loop.  Can you either put it inside the transaction loop,
or produce an explanation as to why this approach is race-free...

Thanks,
Ian.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.