[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v15 2/7] remus: introduce remus device
Sender: xen-devel-bounces@xxxxxxxxxxxxx
On-Behalf-Of: Ian.Jackson@xxxxxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH v15 2/7] remus: introduce remus device
Message-Id: <21438.52873.790154.902193@xxxxxxxxxxxxxxxxxxxxxxxx>
Recipient: ibudiman@xxxxxxxxxxxxx
--- Begin Message ---
Hongyang Yang writes ("Re: [PATCH v15 2/7] remus: introduce remus device"):
> On 07/10/2014 01:26 AM, Ian Jackson wrote:
> > Sorry to mention this now, but I think it would be clearer if all
> > these libxl__remus_device_* functions which manipulate _all_ the
> > devices for a domain used the plural of "device", ie
> > libxl__remus_devices_setup, etc.
>
> Ok
Thanks. You may find git-filter-branch helpful to do this easily.
(Be careful not to blow your leg off.)
> >> + rds->num_devices++;
> >> + /*
> >> + * we add devices that have been setuped to the array no matter
> >> + * the setup process succeed or failed because we need to ensure
> >> + * the device been teardown while setup failed. If any of the
> >> + * device setup failed, we will quit remus, but before we exit,
> >> + * we will teardown the devices that have been added to **dev
> >> + */
> >> + rds->dev[rds->num_set_up++] = dev;
> >> + if (rc) {
> >> + /* setup failed */
> >> + rs->saved_rc = ERROR_FAIL;
> >> + }
> >
> > This doesn't look right. If the setup fails, presumably we shouldn't
> > put the device in the array ? If we do it will presumably be torn
> > down later.
>
> the netbuf script was designed as below:
> 1. when setup failed, the script won't teardown the device itself.
> 2. the teardown op is ok to be excute many times.
Aha. Right.
I think you should state exactly that, probably in a comment here and
also in the script itself. This can replace the comment you have
above, which is rather vague.
> In the remus device layer, if one device setup failed(whether script
> exit or the script get killed or something), we will quit
> remus, but before that, we will teardown all device that has been set
> up, whether it's correctly set up or not. So if we don't put the
> device in the array, we will get a leaked device, that has not been
> teardown.
That makes sense.
> > I still don't understand why libxl__remus_device_state is not part of
> > libxl__remus_state.
>
> libxl__remus_device_state is part of libxl__remus_state:
> +struct libxl__remus_state {
...
> + libxl__remus_device_state dev_state;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I meant: why is it a separate structure, rather than the contents
simply being included there ?
Thanks for your other replies. You don't seem to have replied to
everything I said, including some questions I asked. Do you intend
to, later, perhaps ?
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
--- End Message ---
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
