Re: [libvirt] [PATCH 1/4] esx: parse new URI param 'allow_unsafe'

2014-05-06 10:10 GMT+02:00 Martin Kletzander <mkletzan(a)redhat.com&gt;: > On Mon, May 05, 2014 at 01:48:03PM -0600, Eric Blake wrote: >> >> On 05/05/2014 03:47 AM, Martin Kletzander wrote: >>> >>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >>> --- >>> docs/drvesx.html.in | 14 ++++++++++++++ >>> src/esx/esx_util.c | 13 ++++++++++++- >>> src/esx/esx_util.h | 2 ++ >>> 3 files changed, 28 insertions(+), 1 deletion(-) >>> >>> diff --git a/docs/drvesx.html.in b/docs/drvesx.html.in >>> index 0816baf..d941763 100644 >>> --- a/docs/drvesx.html.in >>> +++ b/docs/drvesx.html.in >>> @@ -185,6 +185,20 @@ >>> vpx://example-vcenter.com/folder1/dc1/folder2/example-esx.com >>> override the default port 1080. >>> </td> >>> </tr> >>> + <tr> >>> + <td> >>> + <code>allow_unsafe</code> >>> + </td> >>> + <td> >>> + <code>0</code> or <code>1</code> >>> + </td> >>> + <td> >>> + If set to 1, this disables check for supported >>> + virtualHW.version, so connections to newer versions >>> + than supported is possible. The default value is 0. >> >> >> "connections" is plural, so s/is possible/are possible/ >> >> On the surface this (and the rest of the series) makes sense - it allows >> older libvirt to be used with a newer ESX version which may or may not >> work, until such time as newer libvirt has been tested to explicitly >> work with that ESX. But I'd like an opinion from someone that actually >> uses esx:// URIs before checking this in. >> > > Since the vmx parsing was moved to vmx/vmx.c (so basically since it > was introduced), we were blindly (or at least in some cases) adding > "support" for newer virtualHW_versions (version 8 in commit 5759a5cc, > 9 in e0eb672e and finally 10 now in 5cc3cce5). I tried getting > someone to test the version I added, but since there's so few of us > working with ESX (I had to ask Matthias if he can have a look at the > code), we can only assume that it will work. I even remember someone > filing a bug that the connection works and everything, but there's a > warning. My thinking was that we have two options here: > > 1) continue messing the code with blindly adding versions of > virtualHW or > > 2) conditionally allow anything the user wants, with the condition > showing that we don't "support" it. > > My reasoning behind choosing version 2 was that the development around > VMX-related backends is not that active as with other ones. So if > anything doesn't work because of HW version it is somehow visible that > the user is not using "supported" versions of vmx descriptors (or how > to call the .vmx file). I designed the patch so there is no > functional nor API change with current usage, it merely allows to > check whether our code works with newer versions without touching the > code (currently, when someone wants to try that, he needs to add the > version to supported ones, obviously). And it still leaves the list > of supported versions in place and whoever wants some version to be > supported can add it to the list. > > I know it is impossible to force people fix everything for versions > they added, but it could be at least a bit visible that we currently > don't have the capacity for that. > > Martin Let's take a step back and look at the complete picture. Initially it seemed to me that the virtualHW version would have a larger effect on the content and format of the VMX file than to actually turned out to have. That's why I added this strict check to the parser in the first place. But after two major vSphere version (4 and 5) and multiple virtualHW version came along since the initial implementation of the VMX parser I'm now pretty confident that the actual virtualHW version is not that important to the parser as I initially thought it would be. I see no gain in adding any extra logic here to allow "unsafe" versions. Actually, I dislike this word in this context, it gives the user a wrong impression about the situation. There is nothing unsafe and nothing to test for compatibility here, it's only me misjudging the importance and meaning of this config entry. I suggest a different approach to deal with future virtualHW versions: basically ignore the value, as it's not that important. It should have been that way from the beginning. Here's a patch for that: https://www.redhat.com/archives/libvir-list/2014-May/msg00313.html