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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Ian Jackson <ian.jackson@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Oleksandr Grytsov <al1img@gmail.com>,
	Oleksandr Grytsov <oleksandr_grytsov@epam.com>,
	"wl@xen.org" <wl@xen.org>
Subject: Re: [Xen-devel] [PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy
Date: Fri, 11 Oct 2019 17:32:14 +0200	[thread overview]
Message-ID: <20191011153214.GL1389@Air-de-Roger.citrite.net> (raw)
In-Reply-To: <23968.39583.655591.751635@mariner.uk.xensource.com>

On Fri, Oct 11, 2019 at 04:07:11PM +0100, Ian Jackson wrote:
> Oleksandr Grytsov writes ("[PATCH v1 2/2] libxl: add removing XS backend path for PV devices on domain destroy"):
> > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > 
> > Currently backend path is remove for specific devices: VBD, VIF, QDISK.
> > This commit adds removing backend path for: VDISPL, VSND, VINPUT.
> 
> Thanks for this and your previous patch.
> 
> This one looks to me like it is probably correct.  But I have not been
> able to understand why this code was limited to vbds and vifs before
> so I am hesitant.
> 
> Searching the git history, I think this has been like this since
> a0eaa86e7537
>  "libxl: add device backend listener in order to launch backends"
> and subsequent commits have just reorganised things but not
> fundamentally changed them.  I've CC'd Roger who wrote this code.

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?

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.

> 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?

I'm not saying it's wrong, it's just that it feels like it belongs in
a different patch maybe.

Thanks, Roger.

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