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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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