Re: [Xen-devel] [PATCH v15 2/7] remus: introduce remus device

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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