[libvirt] [PATCH] qemu: Allow empty script path to <interface/>

Michal Privoznik posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ccf07729b7790e8bcd4d24f42f0d3501e25a8241.1486043388.git.mprivozn@redhat.com
docs/news.xml        | 10 ++++++++++
src/util/virnetdev.c |  4 ++++
2 files changed, 14 insertions(+)
[libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Michal Privoznik 7 years, 1 month ago
Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
have the following interface configuration:

  <interface type='ethernet'/>
    <script path=''/>
  </interface>

This resulted in -netdev tap,script=,.. Fortunately, qemu helped
us to get away with this as it just ignored the empty script
path. However, after the commit mentioned above it's libvirtd
who is executing the script. Unfortunately without special
case-ing empty script path.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/news.xml        | 10 ++++++++++
 src/util/virnetdev.c |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index f408293a1..e7388a8f6 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -56,6 +56,16 @@
           a fabric name has been removed by making it optional.
         </description>
       </change>
+      <change>
+        <summary>
+          qemu: Allow empty script path to &lt;interface/&gt;
+        </summary>
+        <description>
+          Historically, this was always allowed. Unfortunately, due to some
+          rework done for 1.3.2 release a bug was dragged in which suddenly
+          stop allowing domain with such configuration to start.
+        </description>
+      </change>
     </section>
   </release>
   <release version="v3.0.0" date="2017-01-17">
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index fa695d4a6..d12324878 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -2615,6 +2615,10 @@ virNetDevRunEthernetScript(const char *ifname, const char *script)
     virCommandPtr cmd;
     int ret;
 
+    /* Not a bug! Previously we did accept script="" as a NO-OP. */
+    if (STREQ(script, ""))
+        return 0;
+
     cmd = virCommandNew(script);
     virCommandAddArgFormat(cmd, "%s", ifname);
     virCommandClearCaps(cmd);
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Daniel P. Berrange 7 years, 1 month ago
On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
> have the following interface configuration:
> 
>   <interface type='ethernet'/>
>     <script path=''/>
>   </interface>
> 
> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
> us to get away with this as it just ignored the empty script
> path. However, after the commit mentioned above it's libvirtd
> who is executing the script. Unfortunately without special
> case-ing empty script path.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

This was always invalid input and the fact that it happened
to work in the past is just luck. Since this has been broken since
1.3.2 there's plenty of libvirt releases that reject this, and so
any app realistically has to be fixed to omit the <script> entirely
if they want any portability. As such I don't think we need to "fix"
this.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Michal Privoznik 7 years, 1 month ago
On 02/02/2017 02:56 PM, Daniel P. Berrange wrote:
> On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
>> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
>> have the following interface configuration:
>>
>>   <interface type='ethernet'/>
>>     <script path=''/>
>>   </interface>
>>
>> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
>> us to get away with this as it just ignored the empty script
>> path. However, after the commit mentioned above it's libvirtd
>> who is executing the script. Unfortunately without special
>> case-ing empty script path.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> This was always invalid input and the fact that it happened
> to work in the past is just luck. Since this has been broken since
> 1.3.2 there's plenty of libvirt releases that reject this, and so
> any app realistically has to be fixed to omit the <script> entirely
> if they want any portability. As such I don't think we need to "fix"
> this.

Fair enough. Although, 1.3.2 was not that long time ago (~a year). BTW:
it's openstack who is still using this.

https://bugzilla.redhat.com/show_bug.cgi?id=1412834

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Daniel P. Berrange 7 years, 1 month ago
On Thu, Feb 02, 2017 at 04:21:39PM +0100, Michal Privoznik wrote:
> On 02/02/2017 02:56 PM, Daniel P. Berrange wrote:
> > On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
> >> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
> >> have the following interface configuration:
> >>
> >>   <interface type='ethernet'/>
> >>     <script path=''/>
> >>   </interface>
> >>
> >> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
> >> us to get away with this as it just ignored the empty script
> >> path. However, after the commit mentioned above it's libvirtd
> >> who is executing the script. Unfortunately without special
> >> case-ing empty script path.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > 
> > This was always invalid input and the fact that it happened
> > to work in the past is just luck. Since this has been broken since
> > 1.3.2 there's plenty of libvirt releases that reject this, and so
> > any app realistically has to be fixed to omit the <script> entirely
> > if they want any portability. As such I don't think we need to "fix"
> > this.
> 
> Fair enough. Although, 1.3.2 was not that long time ago (~a year). BTW:
> it's openstack who is still using this.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1412834

Seems nova has already been fixed in both master & stable branches

  https://bugs.launchpad.net/nova/+bug/1649527

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Kashyap Chamarthy 7 years, 1 month ago
On Thu, Feb 02, 2017 at 03:29:15PM +0000, Daniel P. Berrange wrote:
> On Thu, Feb 02, 2017 at 04:21:39PM +0100, Michal Privoznik wrote:
> > On 02/02/2017 02:56 PM, Daniel P. Berrange wrote:
> > > On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
> > >> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
> > >> have the following interface configuration:
> > >>
> > >>   <interface type='ethernet'/>
> > >>     <script path=''/>
> > >>   </interface>
> > >>
> > >> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
> > >> us to get away with this as it just ignored the empty script
> > >> path. However, after the commit mentioned above it's libvirtd
> > >> who is executing the script. Unfortunately without special
> > >> case-ing empty script path.
> > >>
> > >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > 
> > > This was always invalid input and the fact that it happened
> > > to work in the past is just luck. Since this has been broken since
> > > 1.3.2 there's plenty of libvirt releases that reject this, and so
> > > any app realistically has to be fixed to omit the <script> entirely
> > > if they want any portability. As such I don't think we need to "fix"
> > > this.
> > 
> > Fair enough. Although, 1.3.2 was not that long time ago (~a year). BTW:
> > it's openstack who is still using this.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1412834

IIUC, this bug was filed to handle the case of _existing_ VMs that were
not launched with the patch that you point to below.

> Seems nova has already been fixed in both master & stable branches
> 
>   https://bugs.launchpad.net/nova/+bug/1649527


-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Daniel P. Berrange 7 years, 1 month ago
On Thu, Feb 02, 2017 at 04:55:29PM +0100, Kashyap Chamarthy wrote:
> On Thu, Feb 02, 2017 at 03:29:15PM +0000, Daniel P. Berrange wrote:
> > On Thu, Feb 02, 2017 at 04:21:39PM +0100, Michal Privoznik wrote:
> > > On 02/02/2017 02:56 PM, Daniel P. Berrange wrote:
> > > > On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
> > > >> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
> > > >> have the following interface configuration:
> > > >>
> > > >>   <interface type='ethernet'/>
> > > >>     <script path=''/>
> > > >>   </interface>
> > > >>
> > > >> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
> > > >> us to get away with this as it just ignored the empty script
> > > >> path. However, after the commit mentioned above it's libvirtd
> > > >> who is executing the script. Unfortunately without special
> > > >> case-ing empty script path.
> > > >>
> > > >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > > 
> > > > This was always invalid input and the fact that it happened
> > > > to work in the past is just luck. Since this has been broken since
> > > > 1.3.2 there's plenty of libvirt releases that reject this, and so
> > > > any app realistically has to be fixed to omit the <script> entirely
> > > > if they want any portability. As such I don't think we need to "fix"
> > > > this.
> > > 
> > > Fair enough. Although, 1.3.2 was not that long time ago (~a year). BTW:
> > > it's openstack who is still using this.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1412834
> 
> IIUC, this bug was filed to handle the case of _existing_ VMs that were
> not launched with the patch that you point to below.

THat is only a problem if you have a running VM on libvirt < 1.3.2 and
you want to migrate it to libvirt >= 1.3.2. That can be solved by
removing the bogus script element prior to migration - nova already
has code that modifies the XML prior to migration where such a fix
could live

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Kashyap Chamarthy 7 years, 1 month ago
On Thu, Feb 02, 2017 at 05:12:54PM +0000, Daniel P. Berrange wrote:
> On Thu, Feb 02, 2017 at 04:55:29PM +0100, Kashyap Chamarthy wrote:

[...]

> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1412834
> > 
> > IIUC, this bug was filed to handle the case of _existing_ VMs that were
> > not launched with the patch that you point to below.
> 
> THat is only a problem if you have a running VM on libvirt < 1.3.2 and
> you want to migrate it to libvirt >= 1.3.2. That can be solved by
> removing the bogus script element prior to migration - nova already
> has code that modifies the XML prior to migration where such a fix
> could live

Ah, that makes sense, the bug can then be closed.  Thanks for clearly
spelling out the only problem scenario.

-- 
/kashyap

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Michal Privoznik 7 years, 1 month ago
On 02/02/2017 06:12 PM, Daniel P. Berrange wrote:
> On Thu, Feb 02, 2017 at 04:55:29PM +0100, Kashyap Chamarthy wrote:
>> On Thu, Feb 02, 2017 at 03:29:15PM +0000, Daniel P. Berrange wrote:
>>> On Thu, Feb 02, 2017 at 04:21:39PM +0100, Michal Privoznik wrote:
>>>> On 02/02/2017 02:56 PM, Daniel P. Berrange wrote:
>>>>> On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
>>>>>> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
>>>>>> have the following interface configuration:
>>>>>>
>>>>>>   <interface type='ethernet'/>
>>>>>>     <script path=''/>
>>>>>>   </interface>
>>>>>>
>>>>>> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
>>>>>> us to get away with this as it just ignored the empty script
>>>>>> path. However, after the commit mentioned above it's libvirtd
>>>>>> who is executing the script. Unfortunately without special
>>>>>> case-ing empty script path.
>>>>>>
>>>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>>>
>>>>> This was always invalid input and the fact that it happened
>>>>> to work in the past is just luck. Since this has been broken since
>>>>> 1.3.2 there's plenty of libvirt releases that reject this, and so
>>>>> any app realistically has to be fixed to omit the <script> entirely
>>>>> if they want any portability. As such I don't think we need to "fix"
>>>>> this.
>>>>
>>>> Fair enough. Although, 1.3.2 was not that long time ago (~a year). BTW:
>>>> it's openstack who is still using this.
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1412834
>>
>> IIUC, this bug was filed to handle the case of _existing_ VMs that were
>> not launched with the patch that you point to below.
> 
> THat is only a problem if you have a running VM on libvirt < 1.3.2 and
> you want to migrate it to libvirt >= 1.3.2. That can be solved by
> removing the bogus script element prior to migration - nova already
> has code that modifies the XML prior to migration where such a fix
> could live

Question is - does it live there already? If not, we should fix it there
too.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Allow empty script path to <interface/>
Posted by Daniel P. Berrange 7 years, 1 month ago
On Thu, Feb 02, 2017 at 02:49:48PM +0100, Michal Privoznik wrote:
> Before 9c17d665fdc5f (v1.3.2 - I know, right?) it was possible to
> have the following interface configuration:
> 
>   <interface type='ethernet'/>
>     <script path=''/>
>   </interface>
> 
> This resulted in -netdev tap,script=,.. Fortunately, qemu helped
> us to get away with this as it just ignored the empty script
> path. However, after the commit mentioned above it's libvirtd
> who is executing the script. Unfortunately without special
> case-ing empty script path.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/news.xml        | 10 ++++++++++
>  src/util/virnetdev.c |  4 ++++
>  2 files changed, 14 insertions(+)

ACK because this causes upgrade problems when using migration. Applications
like OpenStack use migration as their upgrade mechanism. ie to upgrade a new
version of Nova, you would install it on a new compute host and migrate your
guests across. For nova to fix its buggy usage, users would have to upgrade
Nova in-place on existing nodes which is contrary to its normal practice of
using migration for all upgradess


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list