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

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

On 07/11/2014 01:34 AM, Ian Jackson wrote:
> 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.)
Thanks for the tip.

> > > > +    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 ?
Err, I thought I've replied on the last thread about this, but I will
reply here.
I intend to use libxl__remus_state on upper layer, that is, in the
main flow of libxl layer, and use libxl__remus_device_state in both
remus abstract layer and concrete layer. I thought that will make
things more clear. But yes, there still some libxl__remus_state use
in remus abstract layer and concrete layer, I will fix it up in the
next version.

> 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 ?
Sorry, I might have missed some of your comments or questions, but
that's not what I was intend to...I was trying to reply every question
you've pointed out. I will go back and go through your comments carefully.

> Thanks,
> Ian.
> .
--
Thanks,
Yang.

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