Re: [libvirt PATCH v2 01/15] util: properly deal with module vs. driver when binding device to driver

On 11/28/23 8:21 AM, Ján Tomko wrote: Well, without *enough* explanation, it all seems like pointlessly pushing bits around :-). I guess my extra explanation failed though, since people have had issues with the patch twice now). I suppose I can remove it. Reading the paragraph by itself, though, it sounds like 'His name is "Bob", not "*Bob*"'. My spidey senses tell me someone is getting fed up with my verbose explanations :-). I'll try to be more brief. I did send a response, but I sent it after the list switched to the new server, so you won't find it in the old archives (actually when I originally replied I forgot to change the list address, and then had to resend): https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/W... Yes. But you can't assume that the driver name will use dashes, nor even that the driver name will resemble the module name in any programmatically derivable way - although it is common convention that the module name be underscored(drivername), driver name is *not* dashed(modulename). (Technically it is even possible (although I've not seen it) that the driver and module names might be completely unrelated - e.g. module "foo" implementing driver "bar". Sure, we could assume that will never happen, but then in 5 years someone will do that and libvirt 15.2.0 will break because of this assumption made 5 years prior, and some poor soul will have to spend a bunch of time figuring out why.) Yes, that is true. But also: 1) users may know the driver name or the module name, and usually don't know the difference between the two, or even that there are two different names. This blurring between the two has been exacerbated by modprobe always converting "-" into "_", so someone using "vfio-pci" will still automatically get the proper module loaded (because the module just happens to be "vfio_pci"), and binding the new driver will also work properly, but if they use the module name (vfio_pci), then the module will be loaded, but the driver bind will fail. We could just log an error, but "driver 'vfio_pci' not found" is pretty uninformative, and "driver 'vfio_pci' not found - maybe try 'vfio-pci' instead" is kind of pointless - if we already know how to fix the problem, why not just fix it and continue on? (you can bet that if we don't, there will be bug reports saying "Why don't you just automatically fix this instead of just telling me exactly how to fix it?!?!?". As a matter of fact, I'd already had bugs filed because "vfio_pci" wasn't accepted "even though mlx5_vfio_pci is", which is at least part of the origin story of this patch) 2) modules.alias deals only with *module* names, while each device's driver_override file in sysfs only understands *driver* names. And even if we insist that the user specify the driver name, libvirt's own code (see patch 14/15) will be retrieving the *module* name from the modules.alias file, and there will need to be code in libvirt to convert that to driver name in order for the bind operation to be successful. I'm just fixing that in this patch in order to simplify the later patch, and also make user-specified <driver name="blah"/> more foolproof. In the end, all I'm doing is: 1) Making it more clear in libvirt's code where we are using a module name and where we are using a driver name. 1.5) explicitly performing the "dash to underscore" transformation to derive a modulename from the user-provided "driver-module name", but only because we need to follow links in sysfs, and those links *don't* benefit from modprobe's automatic replacement of dash with underscore (i.e. even though we can assume that modprobe will always replace dash with underscore, modprobe isn't the only place that we need the underscore version of the name) 2) following links in sysfs to derive a driver name from a module name, rather than making the *incorrect* assumption that we can get that with dashed(modulename) (which already doesn't work, e.g. for mlx5_vfio_pci/mlx5_vfio_pci) (with current functionality this just automatically fixes a common user error, but beginning with patch 14/15 it will actually be essential to proper functioning of variant driver auto-detect) There is really nothing controversial going on here. Maybe I just spent so much time explaining that everyone *thinks* there must be something fishy.