[BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Philipp Hahn]

[-- Attachment #1.1: Type: text/plain, Size: 1982 bytes --]

Hello,

I have a problem shutting down a domU with xen-4.1.2, which doesn't terminate 
the corresponding blktap2 process, since one (other) VM uses a image file, 
which contains spaces in its file name.
/var/log/xen/xend-debug.log has the following information:

Unhandled exception in thread started by
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", 
line 199, in finishDeviceCleanup
    TapdiskController.destroy(path)
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", 
line 289, in destroy
    tapdisk = TapdiskController.fromDevice(device)
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", 
line 278, in fromDevice
    TapdiskController.list())
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", 
line 256, in list
    key, value = pair.split('=')
ValueError: need more than 1 value to unpack

BlktapController calls "tap-ctl list", which outputs one line for each 
process. Each line contains key=value pairs without any quoting.

# tap-ctl list | grep 'args=.*:.* '
pid=10145 minor=18 state=0 args=aio:/var/lib/libvirt/images/Xen Windows 
drivers (gplpv 308).iso

(passing the output of tap-ctl through a pipe is important, since it switches 
to a different list-format when STDOUT is a tty.)

BlktapController splits the output into lines using \n, then each line at each 
space, and finally each of these 'words' at the '=', which fails for the 
filename.


tap-ctl should probably be changed to use some quoting when printing the 
filename.

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Philipp Hahn]

[-- Attachment #1.1.1: Type: text/plain, Size: 889 bytes --]

Hello,

On Friday 25 November 2011 17:21:08 Philipp Hahn wrote:
> BlktapController splits the output into lines using \n, then each line at
> each space, and finally each of these 'words' at the '=', which fails for
> the filename.

As a quick work-around, the attached patch fixes the problem for me. That is, 
until tap-ctl changes it's output format.
A more permanent solution would be to add proper quoting / escaping to tap-ctl 
and un-quoting / de-escaping  to BlktapController.py

Signed-off-by: Philipp Hahn <hahn@univention.de>

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #1.1.2: 0004-tap-ctl.patch --]
[-- Type: text/x-diff, Size: 2832 bytes --]

Bug #18357: Fix tap-ctl parsing.

Shutting down a domU with xen-4.1.2 doesn't terminate the corresponding blktap2
process, since one (other) VM uses a image file, which contains spaces in its
file name.  /var/log/xen/xend-debug.log has the following information:

Unhandled exception in thread started by
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 199, in finishDeviceCleanup
    TapdiskController.destroy(path)
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 289, in destroy
    tapdisk = TapdiskController.fromDevice(device)
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 278, in fromDevice
    TapdiskController.list())
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 256, in list
    key, value = pair.split('=')
ValueError: need more than 1 value to unpack

BlktapController calls "tap-ctl list", which outputs one line for each process.
Each line contains key=value pairs without any quoting.

# tap-ctl list | grep 'args=.*:.* '
pid=10145 minor=18 state=0 args=aio:/var/lib/libvirt/images/Xen Windows drivers (gplpv 308).iso

(passing the output of tap-ctl through a pipe is important, since it switches
to a different list-format when STDOUT is a tty.)

BlktapController splits the output into lines using \n, then each line at each
space, and finally each of these 'words' at the '=', which fails for the
filename.


Limit the number of splits as a fast work-around.
--- a/tools/python/xen/xend/server/BlktapController.py
+++ b/tools/python/xen/xend/server/BlktapController.py
@@ -249,11 +249,12 @@ class TapdiskController(object):
         _list = TapdiskController.exc('list')
         if not _list: return []
 
-        for line in _list.split('\n'):
+        for line in _list.splitlines():
             tapdisk = TapdiskController.Tapdisk()
 
-            for pair in line.split():
-                key, value = pair.split('=')
+            # Since 'tap-ctl list' does not escape blanks in the path, hard-code the current format using 4 pairs to prevent splitting the path
+            for pair in line.split(None, 4):
+                key, value = pair.split('=', 1)
                 if key == 'pid':
                     tapdisk.pid = value
                 elif key == 'minor':
@@ -264,7 +265,7 @@ class TapdiskController(object):
                 elif key == 'state':
                     tapdisk.state = value
                 elif key == 'args' and value.find(':') != -1:
-                    tapdisk.dtype, tapdisk.image = value.split(':')
+                    tapdisk.dtype, tapdisk.image = value.split(':', 1)
 
             tapdisks.append(tapdisk)
 

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Ian Jackson]

Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py"):
> As a quick work-around, the attached patch fixes the problem for me. That is, 
> until tap-ctl changes it's output format.

Thanks, I have applied it.

> A more permanent solution would be to add proper quoting / escaping to tap-ctl 
> and un-quoting / de-escaping  to BlktapController.py

xend is pretty much obsolete now and I don't imagine we'll be doing
any rework like that soon I'm afraid.

Ian.

Re: [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Pasi Kärkkäinen]

On Thu, Dec 01, 2011 at 06:31:04PM +0000, Ian Jackson wrote:
> Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py"):
> > As a quick work-around, the attached patch fixes the problem for me. That is, 
> > until tap-ctl changes it's output format.
> 
> Thanks, I have applied it.
> 
> > A more permanent solution would be to add proper quoting / escaping to tap-ctl 
> > and un-quoting / de-escaping  to BlktapController.py
> 
> xend is pretty much obsolete now and I don't imagine we'll be doing
> any rework like that soon I'm afraid.
> 

I realize xend doesn't get much attention anymore,
but it's still the only way to use some features,
like: pvusb, pvscsi, remus, block scripts, etc..

So it's good to apply at least bugfix patches :) 

-- Pasi

Re: [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Ian Campbell]

On Thu, 2011-12-01 at 19:37 +0000, Pasi Kärkkäinen wrote:
> On Thu, Dec 01, 2011 at 06:31:04PM +0000, Ian Jackson wrote:
> > Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py"):
> > > As a quick work-around, the attached patch fixes the problem for me. That is, 
> > > until tap-ctl changes it's output format.
> > 
> > Thanks, I have applied it.
> > 
> > > A more permanent solution would be to add proper quoting / escaping to tap-ctl 
> > > and un-quoting / de-escaping  to BlktapController.py
> > 
> > xend is pretty much obsolete now and I don't imagine we'll be doing
> > any rework like that soon I'm afraid.
> > 
> 
> I realize xend doesn't get much attention anymore,
> but it's still the only way to use some features,

xend is deprecated. This ultimately means that xend will go away. If you
are dependent on these features then you need put effort into ensuring
that they become supported in libxl and xl. You should not assume that
xend will be around forever nor that someone else will do this work for
you before xend does go away. In other words people who need these
features need to provide patches for them. (I suppose someone could also
step up and offer to maintain xend properly, I'm not holding my breath
for that).

> like:

> remus, block scripts

AFAIK support for these in xl/libxl these are being worked on.

>  pvusb, pvscsi, 

I'm not aware of any effort to make these work with libxl.

> So it's good to apply at least bugfix patches :) 

TO some extent that just gives people a false sense that they can sit on
xend forever, which is not the case. People need to realise that xend
_is_ going away and they need to start planning accordingly.

> 
> -- Pasi
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [libvirt] [Xen-devel] [BUG] insufficient quoting between " tap-ctl list " and xend/server/BlktapController.py

[Philipp Hahn]

[-- Attachment #1.1: Type: text/plain, Size: 1484 bytes --]

Hello,

On Thursday 01 December 2011 19:31:04 Ian Jackson wrote:
> Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting 
between "tap-ctl list" and xend/server/BlktapController.py"):
> > As a quick work-around, the attached patch fixes the problem for me. That
> > is, until tap-ctl changes it's output format.
>
> Thanks, I have applied it.

Thanks for applying.

On Thursday 01 December 2011 20:51:05 Ian Campbell wrote:
> xend is deprecated. This ultimately means that xend will go away. If you
> are dependent on these features then you need put effort into ensuring
> that they become supported in libxl and xl.

For our forthcomming release of UCS-3.0 we really wanted to switch from Xend 
to xl, but be had to switch back to using Xend, because libvirt still lacks 
some important features like migration; see 
<http://libvirt.org/hvsupport.html> for a comparison matrix. Back at the 
beginning of 2011 Univention sponsored Markus Gross to work on the xl-binding 
of libvirt, but after the end of his internship I have not seen that much 
work on the xl-binding in libvirt except from Jim Fehlig.

Sincerely
Philipp Hahn
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Ian Campbell]

On Fri, 2011-12-02 at 08:51 +0000, Philipp Hahn wrote:
> Hello,
> 
> On Thursday 01 December 2011 19:31:04 Ian Jackson wrote:
> > Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting 
> between "tap-ctl list" and xend/server/BlktapController.py"):
> > > As a quick work-around, the attached patch fixes the problem for me. That
> > > is, until tap-ctl changes it's output format.
> >
> > Thanks, I have applied it.
> 
> Thanks for applying.
> 
> On Thursday 01 December 2011 20:51:05 Ian Campbell wrote:
> > xend is deprecated. This ultimately means that xend will go away. If you
> > are dependent on these features then you need put effort into ensuring
> > that they become supported in libxl and xl.
> 
> For our forthcomming release of UCS-3.0 we really wanted to switch from Xend 
> to xl, but be had to switch back to using Xend, because libvirt still lacks 
> some important features like migration; see 
> <http://libvirt.org/hvsupport.html> for a comparison matrix. Back at the 
> beginning of 2011 Univention sponsored Markus Gross to work on the xl-binding 
> of libvirt, 

Thank you for putting your money where your mouth is here!

> but after the end of his internship I have not seen that much 
> work on the xl-binding in libvirt except from Jim Fehlig.

AFAIK Jim is the main man working on this stuff. Last I heard he was
working on migration but I don't know the status, Jim?

Ian.

Re: [BUG] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Jim Fehlig]

Ian Campbell wrote:
> On Fri, 2011-12-02 at 08:51 +0000, Philipp Hahn wrote:
>   
>> Hello,
>>
>> On Thursday 01 December 2011 19:31:04 Ian Jackson wrote:
>>     
>>> Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting 
>>>       
>> between "tap-ctl list" and xend/server/BlktapController.py"):
>>     
>>>> As a quick work-around, the attached patch fixes the problem for me. That
>>>> is, until tap-ctl changes it's output format.
>>>>         
>>> Thanks, I have applied it.
>>>       
>> Thanks for applying.
>>
>> On Thursday 01 December 2011 20:51:05 Ian Campbell wrote:
>>     
>>> xend is deprecated. This ultimately means that xend will go away. If you
>>> are dependent on these features then you need put effort into ensuring
>>> that they become supported in libxl and xl.
>>>       
>> For our forthcomming release of UCS-3.0 we really wanted to switch from Xend 
>> to xl, but be had to switch back to using Xend, because libvirt still lacks 
>> some important features like migration; see 
>> <http://libvirt.org/hvsupport.html> for a comparison matrix. Back at the 
>> beginning of 2011 Univention sponsored Markus Gross to work on the xl-binding 
>> of libvirt, 
>>     
>
> Thank you for putting your money where your mouth is here!
>
>   
>> but after the end of his internship I have not seen that much 
>> work on the xl-binding in libvirt except from Jim Fehlig.
>>     
>
> AFAIK Jim is the main man working on this stuff. Last I heard he was
> working on migration but I don't know the status, Jim?
>   

As you've probably noticed, I haven't had much time to devote to libxl
and the libxl libvirt driver :-(.  Migration, and some other important
but missing features, are on my todo list, but not quite sure when I'll
get back to this work.

Regards,
Jim

Re: [PATCHv2] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Philipp Hahn]

[-- Attachment #1.1.1: Type: text/plain, Size: 920 bytes --]

Hello Ian,

On Thursday 01 December 2011 19:31:04 you wrote:
> Philipp Hahn writes ("Re: [Xen-devel] [BUG] insufficient quoting 
between "tap-ctl list" and xend/server/BlktapController.py"):
> > As a quick work-around, the attached patch fixes the problem for me. That
> > is, until tap-ctl changes it's output format.
>
> Thanks, I have applied it.

Argh, I submitted the wrong patch. The "line.split(None, 4)" needs to be 
a "3", because 3 splits needs to be done to get the 4 parts.

0004 is the correct full patch, 0005 is the inter-diff.

Sorry for the mixup.

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #1.1.2: 0004-tap-ctl.patch --]
[-- Type: text/x-diff, Size: 2832 bytes --]

Bug #18357: Fix tap-ctl parsing.

Shutting down a domU with xen-4.1.2 doesn't terminate the corresponding blktap2
process, since one (other) VM uses a image file, which contains spaces in its
file name.  /var/log/xen/xend-debug.log has the following information:

Unhandled exception in thread started by
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 199, in finishDeviceCleanup
    TapdiskController.destroy(path)
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 289, in destroy
    tapdisk = TapdiskController.fromDevice(device)
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 278, in fromDevice
    TapdiskController.list())
  File "/usr/lib/python2.6/dist-packages/xen/xend/server/BlktapController.py", line 256, in list
    key, value = pair.split('=')
ValueError: need more than 1 value to unpack

BlktapController calls "tap-ctl list", which outputs one line for each process.
Each line contains key=value pairs without any quoting.

# tap-ctl list | grep 'args=.*:.* '
pid=10145 minor=18 state=0 args=aio:/var/lib/libvirt/images/Xen Windows drivers (gplpv 308).iso

(passing the output of tap-ctl through a pipe is important, since it switches
to a different list-format when STDOUT is a tty.)

BlktapController splits the output into lines using \n, then each line at each
space, and finally each of these 'words' at the '=', which fails for the
filename.


Limit the number of splits as a fast work-around.
--- a/tools/python/xen/xend/server/BlktapController.py
+++ b/tools/python/xen/xend/server/BlktapController.py
@@ -249,11 +249,12 @@ class TapdiskController(object):
         _list = TapdiskController.exc('list')
         if not _list: return []
 
-        for line in _list.split('\n'):
+        for line in _list.splitlines():
             tapdisk = TapdiskController.Tapdisk()
 
-            for pair in line.split():
-                key, value = pair.split('=')
+            # Since 'tap-ctl list' does not escape blanks in the path, hard-code the current format using 4 pairs to prevent splitting the path
+            for pair in line.split(None, 3):
+                key, value = pair.split('=', 1)
                 if key == 'pid':
                     tapdisk.pid = value
                 elif key == 'minor':
@@ -264,7 +265,7 @@ class TapdiskController(object):
                 elif key == 'state':
                     tapdisk.state = value
                 elif key == 'args' and value.find(':') != -1:
-                    tapdisk.dtype, tapdisk.image = value.split(':')
+                    tapdisk.dtype, tapdisk.image = value.split(':', 1)
 
             tapdisks.append(tapdisk)
 

[-- Attachment #1.1.3: 0005-tap-ctl.patch --]
[-- Type: text/x-diff, Size: 785 bytes --]

Bug #18357: Fix tap-ctl parsing.

The previous patch was still wrong: Pythons split() expects the number of
splits, not the number of results.

Do 3 splits to get pid, minor, state and args.
--- a/tools/python/xen/xend/server/BlktapController.py
+++ b/tools/python/xen/xend/server/BlktapController.py
@@ -252,7 +252,7 @@ class TapdiskController(object):
             tapdisk = TapdiskController.Tapdisk()
 
             # Since 'tap-ctl list' does not escape blanks in the path, hard-code the current format using 4 pairs to prevent splitting the path
-            for pair in line.split(None, 4):
+            for pair in line.split(None, 3):
                 key, value = pair.split('=', 1)
                 if key == 'pid':
                     tapdisk.pid = value

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCHv2] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py

[Ian Jackson]

Philipp Hahn writes ("Re: [PATCHv2] insufficient quoting between "tap-ctl list" and xend/server/BlktapController.py"):
> Argh, I submitted the wrong patch. The "line.split(None, 4)" needs to be 
> a "3", because 3 splits needs to be done to get the 4 parts.

Oops.  Thanks for submitting the fix.  I have applied it.

But please remember next time to include a proper signed-off-by.  In
this case, since it was only a single-character change, I applied it
anyway (the interdiff, since the previous version went in already).

Also this kind of thing shows how little review xend patches get
nowadays.  Users of xend are well-advised to switch away before they
trip over some rotting code...

Ian.