[v1] remote: fix driver name check for libxl driver

[libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Daniel P. Berrangé 3 years, 11 months ago

The virConnectGetType() returns "xenlight" for libxl, not "LIBXL".

This prevents users opening a connection to the libxl driver when using
the modular daemons.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/remote/remote_daemon_dispatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 5d1c6971c0..a8ac795d71 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -2111,7 +2111,7 @@ remoteDispatchConnectOpen(virNetServerPtr server G_GNUC_UNUSED,
 
     VIR_DEBUG("Primary driver type is '%s'", type);
     if (STREQ(type, "QEMU") ||
-        STREQ(type, "LIBXL") ||
+        STREQ(type, "xenlight") ||
         STREQ(type, "LXC") ||
         STREQ(type, "VBOX") ||
         STREQ(type, "bhyve") ||
-- 
2.26.2

Re: [libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Jim Fehlig 3 years, 11 months ago

On 5/4/20 10:48 AM, Daniel P. Berrangé wrote:
> The virConnectGetType() returns "xenlight" for libxl, not "LIBXL".

The libxl driver implements connectGetType, where it returns "Xen"

https://gitlab.com/libvirt/libvirt/-/blob/master/src/libxl/libxl_driver.c#L909

Is the driver function table not initialized, in which case virConnectGetType 
returns the driver's name? Either way, I'm really lamenting my choice of names 
and inconsistent use of them in the libxl driver :-(. But I don't think it is 
possible to change the type returned through virConnectGetType, as that could 
break existing users.

Regards,
Jim

> This prevents users opening a connection to the libxl driver when using
> the modular daemons.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/remote/remote_daemon_dispatch.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index 5d1c6971c0..a8ac795d71 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -2111,7 +2111,7 @@ remoteDispatchConnectOpen(virNetServerPtr server G_GNUC_UNUSED,
>   
>       VIR_DEBUG("Primary driver type is '%s'", type);
>       if (STREQ(type, "QEMU") ||
> -        STREQ(type, "LIBXL") ||
> +        STREQ(type, "xenlight") ||
>           STREQ(type, "LXC") ||
>           STREQ(type, "VBOX") ||
>           STREQ(type, "bhyve") ||
>

Re: [libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Daniel P. Berrangé 3 years, 11 months ago

On Mon, May 04, 2020 at 11:33:46AM -0600, Jim Fehlig wrote:
> On 5/4/20 10:48 AM, Daniel P. Berrangé wrote:
> > The virConnectGetType() returns "xenlight" for libxl, not "LIBXL".
> 
> The libxl driver implements connectGetType, where it returns "Xen"
> 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/libxl/libxl_driver.c#L909
> 
> Is the driver function table not initialized, in which case
> virConnectGetType returns the driver's name? Either way, I'm really
> lamenting my choice of names and inconsistent use of them in the libxl
> driver :-(. But I don't think it is possible to change the type returned
> through virConnectGetType, as that could break existing users.

Doh, I missed that. So we have virHypervisorDriver using "xenlight",
and virStateDriver using  "LIBXL" and virConnectGetType using "Xen".

Can we changes the driver tables to use "Xen" too, now that we got
rid of the old Xen driver. I can't remember if the driver tables
names leak out anywhere important ?

> 
> Regards,
> Jim
> 
> > This prevents users opening a connection to the libxl driver when using
> > the modular daemons.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   src/remote/remote_daemon_dispatch.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> > index 5d1c6971c0..a8ac795d71 100644
> > --- a/src/remote/remote_daemon_dispatch.c
> > +++ b/src/remote/remote_daemon_dispatch.c
> > @@ -2111,7 +2111,7 @@ remoteDispatchConnectOpen(virNetServerPtr server G_GNUC_UNUSED,
> >       VIR_DEBUG("Primary driver type is '%s'", type);
> >       if (STREQ(type, "QEMU") ||
> > -        STREQ(type, "LIBXL") ||
> > +        STREQ(type, "xenlight") ||
> >           STREQ(type, "LXC") ||
> >           STREQ(type, "VBOX") ||
> >           STREQ(type, "bhyve") ||
> > 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Jim Fehlig 3 years, 11 months ago

On 5/4/20 11:43 AM, Daniel P. Berrangé wrote:
> On Mon, May 04, 2020 at 11:33:46AM -0600, Jim Fehlig wrote:
>> On 5/4/20 10:48 AM, Daniel P. Berrangé wrote:
>>> The virConnectGetType() returns "xenlight" for libxl, not "LIBXL".
>>
>> The libxl driver implements connectGetType, where it returns "Xen"
>>
>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/libxl/libxl_driver.c#L909
>>
>> Is the driver function table not initialized, in which case
>> virConnectGetType returns the driver's name? Either way, I'm really
>> lamenting my choice of names and inconsistent use of them in the libxl
>> driver :-(. But I don't think it is possible to change the type returned
>> through virConnectGetType, as that could break existing users.
> 
> Doh, I missed that. So we have virHypervisorDriver using "xenlight",
> and virStateDriver using  "LIBXL" and virConnectGetType using "Xen".
> 
> Can we changes the driver tables to use "Xen" too, now that we got
> rid of the old Xen driver. I can't remember if the driver tables
> names leak out anywhere important ?

It doesn't appear to be the case, but I only looked quickly and could have 
missed something.

I'll send a patch to change 'name' in the driver tables. With the old Xen driver 
gone Andrea and I also discussed renaming the libxl driver to xen, but I'm not 
sure it is worth the effort. It seems difficult to remove all references to 
libxl. E.g. changing the '--with-libxl' configure option would break external 
users/scripts.

Regards,
Jim

Re: [libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Andrea Bolognani 3 years, 11 months ago

On Mon, 2020-05-04 at 12:10 -0600, Jim Fehlig wrote:
> On 5/4/20 11:43 AM, Daniel P. Berrangé wrote:
> > Can we changes the driver tables to use "Xen" too, now that we got
> > rid of the old Xen driver. I can't remember if the driver tables
> > names leak out anywhere important ?
> 
> It doesn't appear to be the case, but I only looked quickly and could have 
> missed something.
> 
> I'll send a patch to change 'name' in the driver tables. With the old Xen driver 
> gone Andrea and I also discussed renaming the libxl driver to xen, but I'm not 
> sure it is worth the effort. It seems difficult to remove all references to 
> libxl. E.g. changing the '--with-libxl' configure option would break external 
> users/scripts.

I think it's fine to make changes to configure options, as long as
passing the old options results in a failure rather than silently
(not) enabling the driver. Our API stability guarantee thankfully
does *not* extend to configure :)

On the other hand, Any Minute Now™ we're going to switch to Meson,
at which point *all* of the existing options will break and all of
the scripts that exist will have to be updated. Maybe we can avoid
doing anything to autotools and just make sure the corresponding
Meson option is named correctly from day one?

Adding Pavel to the conversation.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Pavel Hrdina 3 years, 11 months ago

On Tue, May 05, 2020 at 12:37:58PM +0200, Andrea Bolognani wrote:
> On Mon, 2020-05-04 at 12:10 -0600, Jim Fehlig wrote:
> > On 5/4/20 11:43 AM, Daniel P. Berrangé wrote:
> > > Can we changes the driver tables to use "Xen" too, now that we got
> > > rid of the old Xen driver. I can't remember if the driver tables
> > > names leak out anywhere important ?
> > 
> > It doesn't appear to be the case, but I only looked quickly and could have 
> > missed something.
> > 
> > I'll send a patch to change 'name' in the driver tables. With the old Xen driver 
> > gone Andrea and I also discussed renaming the libxl driver to xen, but I'm not 
> > sure it is worth the effort. It seems difficult to remove all references to 
> > libxl. E.g. changing the '--with-libxl' configure option would break external 
> > users/scripts.
> 
> I think it's fine to make changes to configure options, as long as
> passing the old options results in a failure rather than silently
> (not) enabling the driver. Our API stability guarantee thankfully
> does *not* extend to configure :)
> 
> On the other hand, Any Minute Now™ we're going to switch to Meson,
> at which point *all* of the existing options will break and all of
> the scripts that exist will have to be updated. Maybe we can avoid
> doing anything to autotools and just make sure the corresponding
> Meson option is named correctly from day one?

I would probably change the name beforehand in configure to make the
transition to Meson as direct as possible for the sake of reviewers.
It can be done within the same release as the switch to meson to
minimize impact to all building scripts.

Pavel

Re: [libvirt PATCH] remote: fix driver name check for libxl driver

Posted by Daniel P. Berrangé 3 years, 11 months ago

On Tue, May 05, 2020 at 12:46:16PM +0200, Pavel Hrdina wrote:
> On Tue, May 05, 2020 at 12:37:58PM +0200, Andrea Bolognani wrote:
> > On Mon, 2020-05-04 at 12:10 -0600, Jim Fehlig wrote:
> > > On 5/4/20 11:43 AM, Daniel P. Berrangé wrote:
> > > > Can we changes the driver tables to use "Xen" too, now that we got
> > > > rid of the old Xen driver. I can't remember if the driver tables
> > > > names leak out anywhere important ?
> > > 
> > > It doesn't appear to be the case, but I only looked quickly and could have 
> > > missed something.
> > > 
> > > I'll send a patch to change 'name' in the driver tables. With the old Xen driver 
> > > gone Andrea and I also discussed renaming the libxl driver to xen, but I'm not 
> > > sure it is worth the effort. It seems difficult to remove all references to 
> > > libxl. E.g. changing the '--with-libxl' configure option would break external 
> > > users/scripts.
> > 
> > I think it's fine to make changes to configure options, as long as
> > passing the old options results in a failure rather than silently
> > (not) enabling the driver. Our API stability guarantee thankfully
> > does *not* extend to configure :)
> > 
> > On the other hand, Any Minute Now™ we're going to switch to Meson,
> > at which point *all* of the existing options will break and all of
> > the scripts that exist will have to be updated. Maybe we can avoid
> > doing anything to autotools and just make sure the corresponding
> > Meson option is named correctly from day one?
> 
> I would probably change the name beforehand in configure to make the
> transition to Meson as direct as possible for the sake of reviewers.
> It can be done within the same release as the switch to meson to
> minimize impact to all building scripts.

Yeah, that makes sense to me.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|