Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy

From: Oleksandr Grytsov <al1img@gmail.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@citrix.com>,
	Oleksandr Grytsov <oleksandr_grytsov@epam.com>,
	"wl@xen.org" <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
Date: Wed, 16 Oct 2019 13:39:28 +0300	[thread overview]
Message-ID: <CACvf2oWcTQDJ-ZBVFXJM9055mDRfkD8KC2+3CkkhsuBr6a5+mQ@mail.gmail.com> (raw)
In-Reply-To: <20191015153947.GA17494@Air-de-Roger>

On Tue, Oct 15, 2019 at 6:39 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Fri, Oct 11, 2019 at 04:55:32PM +0100, Ian Jackson wrote:
> > Roger Pau Monne writes ("Re: [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > > When this code was added (devd) those where the only backends handled
> > > by libxl. VDISPL, VSND, VINPUT didn't exist at that point, so I think
> > > the issue is that when support for those was added, it wasn't properly
> > > wired into devd.
> > >
> > > > I think:
> > > >
> > > > >      switch(ddev->dev->backend_kind) {
> > > > > +    case LIBXL__DEVICE_KIND_VDISPL:
> > > > > +    case LIBXL__DEVICE_KIND_VSND:
> > > > > +    case LIBXL__DEVICE_KIND_VINPUT:
> > > > >      case LIBXL__DEVICE_KIND_VBD:
> > > > >      case LIBXL__DEVICE_KIND_VIF:
> > > >
> > > > If we do want this to handle *all* kinds of device, maybe it should
> > > > have a fallback that aborts, or something ?  (I don't think it is
> > > > easy to make it a compile error to forget to add an entry here but if
> > > > we could do that it would probably be best.)
> > >
> > > Maybe we could have some generic handling for everything != qdisk?
> >
> > So make that the "default:" ?  That would be fine by me.
>
> If possible yes, that would be my preference, and would prevent having
> to add new device types to this switch unless special handling is
> required.
>
> >
> > > IIRC qdisk needs special handling so that devd can launch and destroy
> > > a QEMU instance when required, but other backends that run in the
> > > kernel don't need such handling and could maybe share the same generic
> > > code path.
> >
> > Right.
> >
> > > > All of that assuming that the basic idea is right, which I would like
> > > > Roger's opinion about.
> > >
> > > Looking at the patch itself, you also seem to be doing some changes
> > > related to num_vbds and num_vifs, but those are not mentioned in the
> > > commit message, is that a stray change?
> >
> > No, I don't think so.  Those variables just tell us when the thing is
> > torn down but Oleksandr's code is able to use the devices slist itself
> > for that.  Please do check to see if you agree.
>
> Yes, that's fine. I don't think those changes are wrong, I just think
> that at least they should be mentioned in the commit message. Ie:
> "while there remove the num_vifs and num_vbds since they are not
> needed and the same can be achieved by checking that the device list
> is empty." or some such.
>
> Thanks, Roger.

Ian, Roger,

Thanks for reviewing and comments. I will update the patch with your
comments above.

-- 
Best Regards,
Oleksandr Grytsov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel