[PATCH 0/3] scripts/qmp/qom-fuse: Scrape off the bitrot

[PATCH 0/3] scripts/qmp/qom-fuse: Scrape off the bitrot

[Markus Armbruster]

Markus Armbruster (3):
  scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol
  scripts/qmp/qom-fuse: Port to current Python module fuse
  scripts/qmp/qom-fuse: Fix getattr(), read() for files in /

 scripts/qmp/qom-fuse | 107 +++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 50 deletions(-)

-- 
2.26.2

[PATCH 1/3] scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol

[Markus Armbruster]

Commit c7b942d7f8 "scripts/qmp: Fix shebang and imports" messed with
it for reasons I don't quite understand.  I do understand how it fails
now: it neglects to import sys.  Fix that.

It now fails because it expects an old version of module fuse.  That's
next.

Fixes: c7b942d7f84ef54f266921bf7668d43f1f2c7c79
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qmp/qom-fuse | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index 5fa6b3bf64..b7dabe8d65 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -13,7 +13,7 @@
 
 import fuse, stat
 from fuse import Fuse
-import os, posix
+import os, posix, sys
 from errno import *
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
@@ -134,7 +134,7 @@ class QOMFS(Fuse):
             yield fuse.Direntry(str(item['name']))
 
 if __name__ == '__main__':
-    import sys, os
+    import os
 
     fs = QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET']))
     fs.main(sys.argv)
-- 
2.26.2

Re: [PATCH 1/3] scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol

[Philippe Mathieu-Daudé]

On 7/23/20 4:27 PM, Markus Armbruster wrote:
> Commit c7b942d7f8 "scripts/qmp: Fix shebang and imports" messed with
> it for reasons I don't quite understand.  I do understand how it fails
> now: it neglects to import sys.  Fix that.

Oops I missed that.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> It now fails because it expects an old version of module fuse.  That's
> next.
> 
> Fixes: c7b942d7f84ef54f266921bf7668d43f1f2c7c79
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qmp/qom-fuse | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
> index 5fa6b3bf64..b7dabe8d65 100755
> --- a/scripts/qmp/qom-fuse
> +++ b/scripts/qmp/qom-fuse
> @@ -13,7 +13,7 @@
>  
>  import fuse, stat
>  from fuse import Fuse
> -import os, posix
> +import os, posix, sys
>  from errno import *
>  
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> @@ -134,7 +134,7 @@ class QOMFS(Fuse):
>              yield fuse.Direntry(str(item['name']))
>  
>  if __name__ == '__main__':
> -    import sys, os
> +    import os
>  
>      fs = QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET']))
>      fs.main(sys.argv)
> 

Re: [PATCH 1/3] scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol

[John Snow]

On 7/23/20 10:27 AM, Markus Armbruster wrote:
> Commit c7b942d7f8 "scripts/qmp: Fix shebang and imports" messed with
> it for reasons I don't quite understand.  I do understand how it fails
> now: it neglects to import sys.  Fix that.
> 

Apologies. These scripts didn't appear to work because they don't have 
any clue where the script they are trying to import lives. I was working 
on a series that refactored ./python/qemu into a python package.

The back half of that series hasn't landed upstream yet, so the import 
refuddling looks an awful lot more arbitrary at the moment, but the idea 
is that the scripts SHOULD work without needing to explicitly set your 
PYTHONPATH. For the moment, I think that's better.

My ultimate end-game is to get most python scripts under ./python/ and 
checked with pylint/mypy etc. as it will help detect breaking changes if 
library routines change. I want to institute a tree-wide regime for 
python code management that has a unified vision about how imports work 
and so on.

I would hope that this would reduce confusion in the future about how to 
execute scripts, how to write import statements, etc.

Most of what I am doing is baby steps towards that.

> It now fails because it expects an old version of module fuse.  That's
> next.
> 

See also my commit message: "There's more wrong with these scripts; ..."

> Fixes: c7b942d7f84ef54f266921bf7668d43f1f2c7c79
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks:

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>   scripts/qmp/qom-fuse | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
> index 5fa6b3bf64..b7dabe8d65 100755
> --- a/scripts/qmp/qom-fuse
> +++ b/scripts/qmp/qom-fuse
> @@ -13,7 +13,7 @@
>   
>   import fuse, stat
>   from fuse import Fuse
> -import os, posix
> +import os, posix, sys
>   from errno import *
>   
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> @@ -134,7 +134,7 @@ class QOMFS(Fuse):
>               yield fuse.Direntry(str(item['name']))
>   
>   if __name__ == '__main__':
> -    import sys, os
> +    import os
>   
>       fs = QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET']))
>       fs.main(sys.argv)
> 

[PATCH 2/3] scripts/qmp/qom-fuse: Port to current Python module fuse

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qmp/qom-fuse | 93 ++++++++++++++++++++++----------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index b7dabe8d65..405e6ebd67 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -3,16 +3,18 @@
 # QEMU Object Model test tools
 #
 # Copyright IBM, Corp. 2012
+# Copyright (C) 2020 Red Hat, Inc.
 #
 # Authors:
 #  Anthony Liguori   <aliguori@us.ibm.com>
+#  Markus Armbruster <armbru@redhat.com>
 #
 # This work is licensed under the terms of the GNU GPL, version 2 or later.  See
 # the COPYING file in the top-level directory.
 ##
 
 import fuse, stat
-from fuse import Fuse
+from fuse import FUSE, FuseOSError, Operations
 import os, posix, sys
 from errno import *
 
@@ -21,9 +23,8 @@ from qemu.qmp import QEMUMonitorProtocol
 
 fuse.fuse_python_api = (0, 2)
 
-class QOMFS(Fuse):
-    def __init__(self, qmp, *args, **kwds):
-        Fuse.__init__(self, *args, **kwds)
+class QOMFS(Operations):
+    def __init__(self, qmp):
         self.qmp = qmp
         self.qmp.connect()
         self.ino_map = {}
@@ -65,21 +66,21 @@ class QOMFS(Fuse):
         except:
             return False
 
-    def read(self, path, length, offset):
+    def read(self, path, length, offset, fh):
         if not self.is_property(path):
             return -ENOENT
 
         path, prop = path.rsplit('/', 1)
         try:
-            data = str(self.qmp.command('qom-get', path=path, property=prop))
+            data = self.qmp.command('qom-get', path=path, property=prop)
             data += '\n' # make values shell friendly
         except:
-            return -EPERM
+            raise FuseOSError(EPERM)
 
         if offset > len(data):
             return ''
 
-        return str(data[offset:][:length])
+        return bytes(data[offset:][:length], encoding='utf-8')
 
     def readlink(self, path):
         if not self.is_link(path):
@@ -89,52 +90,52 @@ class QOMFS(Fuse):
         return prefix + str(self.qmp.command('qom-get', path=path,
                                              property=prop))
 
-    def getattr(self, path):
+    def getattr(self, path, fh=None):
         if self.is_link(path):
-            value = posix.stat_result((0o755 | stat.S_IFLNK,
-                                       self.get_ino(path),
-                                       0,
-                                       2,
-                                       1000,
-                                       1000,
-                                       4096,
-                                       0,
-                                       0,
-                                       0))
+            value = { 'st_mode': 0o755 | stat.S_IFLNK,
+                      'st_ino': self.get_ino(path),
+                      'st_dev': 0,
+                      'st_nlink': 2,
+                      'st_uid': 1000,
+                      'st_gid': 1000,
+                      'st_size': 4096,
+                      'st_atime': 0,
+                      'st_mtime': 0,
+                      'st_ctime': 0 }
         elif self.is_object(path):
-            value = posix.stat_result((0o755 | stat.S_IFDIR,
-                                       self.get_ino(path),
-                                       0,
-                                       2,
-                                       1000,
-                                       1000,
-                                       4096,
-                                       0,
-                                       0,
-                                       0))
+            value = { 'st_mode': 0o755 | stat.S_IFDIR,
+                      'st_ino': self.get_ino(path),
+                      'st_dev': 0,
+                      'st_nlink': 2,
+                      'st_uid': 1000,
+                      'st_gid': 1000,
+                      'st_size': 4096,
+                      'st_atime': 0,
+                      'st_mtime': 0,
+                      'st_ctime': 0 }
         elif self.is_property(path):
-            value = posix.stat_result((0o644 | stat.S_IFREG,
-                                       self.get_ino(path),
-                                       0,
-                                       1,
-                                       1000,
-                                       1000,
-                                       4096,
-                                       0,
-                                       0,
-                                       0))
+            value = { 'st_mode': 0o644 | stat.S_IFREG,
+                      'st_ino': self.get_ino(path),
+                      'st_dev': 0,
+                      'st_nlink': 1,
+                      'st_uid': 1000,
+                      'st_gid': 1000,
+                      'st_size': 4096,
+                      'st_atime': 0,
+                      'st_mtime': 0,
+                      'st_ctime': 0 }
         else:
-            value = -ENOENT
+            raise FuseOSError(ENOENT)
         return value
 
-    def readdir(self, path, offset):
-        yield fuse.Direntry('.')
-        yield fuse.Direntry('..')
+    def readdir(self, path, fh):
+        yield '.'
+        yield '..'
         for item in self.qmp.command('qom-list', path=path):
-            yield fuse.Direntry(str(item['name']))
+            yield str(item['name'])
 
 if __name__ == '__main__':
     import os
 
-    fs = QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET']))
-    fs.main(sys.argv)
+    fuse = FUSE(QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET'])),
+                sys.argv[1], foreground=True)
-- 
2.26.2

Re: [PATCH 2/3] scripts/qmp/qom-fuse: Port to current Python module fuse

[John Snow]

On 7/23/20 10:27 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Honestly, benefit of the doubt on this one. The Python looks fine, but I 
don't know much about the FUSE module. Still, it was broken before, so 
if you claim it now works for you, that's more useful than it used to be.

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>   scripts/qmp/qom-fuse | 93 ++++++++++++++++++++++----------------------
>   1 file changed, 47 insertions(+), 46 deletions(-)
> 
> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
> index b7dabe8d65..405e6ebd67 100755
> --- a/scripts/qmp/qom-fuse
> +++ b/scripts/qmp/qom-fuse
> @@ -3,16 +3,18 @@
>   # QEMU Object Model test tools
>   #
>   # Copyright IBM, Corp. 2012
> +# Copyright (C) 2020 Red Hat, Inc.
>   #
>   # Authors:
>   #  Anthony Liguori   <aliguori@us.ibm.com>
> +#  Markus Armbruster <armbru@redhat.com>
>   #
>   # This work is licensed under the terms of the GNU GPL, version 2 or later.  See
>   # the COPYING file in the top-level directory.
>   ##
>   
>   import fuse, stat
> -from fuse import Fuse
> +from fuse import FUSE, FuseOSError, Operations
>   import os, posix, sys
>   from errno import *
>   
> @@ -21,9 +23,8 @@ from qemu.qmp import QEMUMonitorProtocol
>   
>   fuse.fuse_python_api = (0, 2)
>   
> -class QOMFS(Fuse):
> -    def __init__(self, qmp, *args, **kwds):
> -        Fuse.__init__(self, *args, **kwds)
> +class QOMFS(Operations):
> +    def __init__(self, qmp):
>           self.qmp = qmp
>           self.qmp.connect()
>           self.ino_map = {}
> @@ -65,21 +66,21 @@ class QOMFS(Fuse):
>           except:
>               return False
>   
> -    def read(self, path, length, offset):
> +    def read(self, path, length, offset, fh):
>           if not self.is_property(path):
>               return -ENOENT
>   
>           path, prop = path.rsplit('/', 1)
>           try:
> -            data = str(self.qmp.command('qom-get', path=path, property=prop))
> +            data = self.qmp.command('qom-get', path=path, property=prop)
>               data += '\n' # make values shell friendly
>           except:
> -            return -EPERM
> +            raise FuseOSError(EPERM)
>   
>           if offset > len(data):
>               return ''
>   
> -        return str(data[offset:][:length])
> +        return bytes(data[offset:][:length], encoding='utf-8')
>   
>       def readlink(self, path):
>           if not self.is_link(path):
> @@ -89,52 +90,52 @@ class QOMFS(Fuse):
>           return prefix + str(self.qmp.command('qom-get', path=path,
>                                                property=prop))
>   
> -    def getattr(self, path):
> +    def getattr(self, path, fh=None):
>           if self.is_link(path):
> -            value = posix.stat_result((0o755 | stat.S_IFLNK,
> -                                       self.get_ino(path),
> -                                       0,
> -                                       2,
> -                                       1000,
> -                                       1000,
> -                                       4096,
> -                                       0,
> -                                       0,
> -                                       0))
> +            value = { 'st_mode': 0o755 | stat.S_IFLNK,
> +                      'st_ino': self.get_ino(path),
> +                      'st_dev': 0,
> +                      'st_nlink': 2,
> +                      'st_uid': 1000,
> +                      'st_gid': 1000,
> +                      'st_size': 4096,
> +                      'st_atime': 0,
> +                      'st_mtime': 0,
> +                      'st_ctime': 0 }
>           elif self.is_object(path):
> -            value = posix.stat_result((0o755 | stat.S_IFDIR,
> -                                       self.get_ino(path),
> -                                       0,
> -                                       2,
> -                                       1000,
> -                                       1000,
> -                                       4096,
> -                                       0,
> -                                       0,
> -                                       0))
> +            value = { 'st_mode': 0o755 | stat.S_IFDIR,
> +                      'st_ino': self.get_ino(path),
> +                      'st_dev': 0,
> +                      'st_nlink': 2,
> +                      'st_uid': 1000,
> +                      'st_gid': 1000,
> +                      'st_size': 4096,
> +                      'st_atime': 0,
> +                      'st_mtime': 0,
> +                      'st_ctime': 0 }
>           elif self.is_property(path):
> -            value = posix.stat_result((0o644 | stat.S_IFREG,
> -                                       self.get_ino(path),
> -                                       0,
> -                                       1,
> -                                       1000,
> -                                       1000,
> -                                       4096,
> -                                       0,
> -                                       0,
> -                                       0))
> +            value = { 'st_mode': 0o644 | stat.S_IFREG,
> +                      'st_ino': self.get_ino(path),
> +                      'st_dev': 0,
> +                      'st_nlink': 1,
> +                      'st_uid': 1000,
> +                      'st_gid': 1000,
> +                      'st_size': 4096,
> +                      'st_atime': 0,
> +                      'st_mtime': 0,
> +                      'st_ctime': 0 }
>           else:
> -            value = -ENOENT
> +            raise FuseOSError(ENOENT)
>           return value
>   
> -    def readdir(self, path, offset):
> -        yield fuse.Direntry('.')
> -        yield fuse.Direntry('..')
> +    def readdir(self, path, fh):
> +        yield '.'
> +        yield '..'
>           for item in self.qmp.command('qom-list', path=path):
> -            yield fuse.Direntry(str(item['name']))
> +            yield str(item['name'])
>   
>   if __name__ == '__main__':
>       import os
>   
> -    fs = QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET']))
> -    fs.main(sys.argv)
> +    fuse = FUSE(QOMFS(QEMUMonitorProtocol(os.environ['QMP_SOCKET'])),
> +                sys.argv[1], foreground=True)
> 

Re: [PATCH 2/3] scripts/qmp/qom-fuse: Port to current Python module fuse

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 7/23/20 10:27 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Honestly, benefit of the doubt on this one. The Python looks fine, but
> I don't know much about the FUSE module.

All I knew when I started this job was that the script had worked for me
before with some Fedora Python fuse package, and didn't work with Fedora
32's python3-fusepy.

>                                          Still, it was broken before,
> so if you claim it now works for you, that's more useful than it used
> to be.
>
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!

[PATCH 3/3] scripts/qmp/qom-fuse: Fix getattr(), read() for files in /

[Markus Armbruster]

path, prop = "type".rsplit('/', 1) sets path to "", which doesn't
work.  Correct to "/".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qmp/qom-fuse | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index 405e6ebd67..7c7cff8edf 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -45,8 +45,10 @@ class QOMFS(Operations):
             return False
 
     def is_property(self, path):
+        path, prop = path.rsplit('/', 1)
+        if path == '':
+            path = '/'
         try:
-            path, prop = path.rsplit('/', 1)
             for item in self.qmp.command('qom-list', path=path):
                 if item['name'] == prop:
                     return True
@@ -55,8 +57,10 @@ class QOMFS(Operations):
             return False
 
     def is_link(self, path):
+        path, prop = path.rsplit('/', 1)
+        if path == '':
+            path = '/'
         try:
-            path, prop = path.rsplit('/', 1)
             for item in self.qmp.command('qom-list', path=path):
                 if item['name'] == prop:
                     if item['type'].startswith('link<'):
@@ -71,6 +75,8 @@ class QOMFS(Operations):
             return -ENOENT
 
         path, prop = path.rsplit('/', 1)
+        if path == '':
+            path = '/'
         try:
             data = self.qmp.command('qom-get', path=path, property=prop)
             data += '\n' # make values shell friendly
-- 
2.26.2

Re: [PATCH 3/3] scripts/qmp/qom-fuse: Fix getattr(), read() for files in /

[Philippe Mathieu-Daudé]

On 7/23/20 4:27 PM, Markus Armbruster wrote:
> path, prop = "type".rsplit('/', 1) sets path to "", which doesn't
> work.  Correct to "/".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qmp/qom-fuse | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
> index 405e6ebd67..7c7cff8edf 100755
> --- a/scripts/qmp/qom-fuse
> +++ b/scripts/qmp/qom-fuse
> @@ -45,8 +45,10 @@ class QOMFS(Operations):
>              return False
>  
>      def is_property(self, path):
> +        path, prop = path.rsplit('/', 1)
> +        if path == '':
> +            path = '/'
>          try:
> -            path, prop = path.rsplit('/', 1)

Maybe worth adding an tiny root_path_split() helper?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>              for item in self.qmp.command('qom-list', path=path):
>                  if item['name'] == prop:
>                      return True
> @@ -55,8 +57,10 @@ class QOMFS(Operations):
>              return False
>  
>      def is_link(self, path):
> +        path, prop = path.rsplit('/', 1)
> +        if path == '':
> +            path = '/'
>          try:
> -            path, prop = path.rsplit('/', 1)
>              for item in self.qmp.command('qom-list', path=path):
>                  if item['name'] == prop:
>                      if item['type'].startswith('link<'):
> @@ -71,6 +75,8 @@ class QOMFS(Operations):
>              return -ENOENT
>  
>          path, prop = path.rsplit('/', 1)
> +        if path == '':
> +            path = '/'
>          try:
>              data = self.qmp.command('qom-get', path=path, property=prop)
>              data += '\n' # make values shell friendly
> 

Re: [PATCH 3/3] scripts/qmp/qom-fuse: Fix getattr(), read() for files in /

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/23/20 4:27 PM, Markus Armbruster wrote:
>> path, prop = "type".rsplit('/', 1) sets path to "", which doesn't
>> work.  Correct to "/".
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qmp/qom-fuse | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
>> index 405e6ebd67..7c7cff8edf 100755
>> --- a/scripts/qmp/qom-fuse
>> +++ b/scripts/qmp/qom-fuse
>> @@ -45,8 +45,10 @@ class QOMFS(Operations):
>>              return False
>>  
>>      def is_property(self, path):
>> +        path, prop = path.rsplit('/', 1)
>> +        if path == '':
>> +            path = '/'
>>          try:
>> -            path, prop = path.rsplit('/', 1)
>
> Maybe worth adding an tiny root_path_split() helper?

The script goes back to commit 5ade767485 "qom: quick and dirty QOM
filesystem based on FUSE" (2014).  It's as "quick and dirty" as ever.
It could use a thorough rework.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

[...]

Re: [PATCH 3/3] scripts/qmp/qom-fuse: Fix getattr(), read() for files in /

[John Snow]

On 7/23/20 10:27 AM, Markus Armbruster wrote:
> path, prop = "type".rsplit('/', 1) sets path to "", which doesn't
> work.  Correct to "/".
> 

BOTD. If it works for you, that's good news.

Reviewed-by: John Snow <jsnow@redhat.com>

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qmp/qom-fuse | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
> index 405e6ebd67..7c7cff8edf 100755
> --- a/scripts/qmp/qom-fuse
> +++ b/scripts/qmp/qom-fuse
> @@ -45,8 +45,10 @@ class QOMFS(Operations):
>               return False
>   
>       def is_property(self, path):
> +        path, prop = path.rsplit('/', 1)
> +        if path == '':
> +            path = '/'
>           try:
> -            path, prop = path.rsplit('/', 1)
>               for item in self.qmp.command('qom-list', path=path):
>                   if item['name'] == prop:
>                       return True
> @@ -55,8 +57,10 @@ class QOMFS(Operations):
>               return False
>   
>       def is_link(self, path):
> +        path, prop = path.rsplit('/', 1)
> +        if path == '':
> +            path = '/'
>           try:
> -            path, prop = path.rsplit('/', 1)
>               for item in self.qmp.command('qom-list', path=path):
>                   if item['name'] == prop:
>                       if item['type'].startswith('link<'):
> @@ -71,6 +75,8 @@ class QOMFS(Operations):
>               return -ENOENT
>   
>           path, prop = path.rsplit('/', 1)
> +        if path == '':
> +            path = '/'
>           try:
>               data = self.qmp.command('qom-get', path=path, property=prop)
>               data += '\n' # make values shell friendly
> 

Re: [PATCH 0/3] scripts/qmp/qom-fuse: Scrape off the bitrot

[Thomas Huth]

On 23/07/2020 16.27, Markus Armbruster wrote:
> Markus Armbruster (3):
>   scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol
>   scripts/qmp/qom-fuse: Port to current Python module fuse
>   scripts/qmp/qom-fuse: Fix getattr(), read() for files in /

Could it be added to a CI pipeline, so that it does not bitrot again?

 Thomas

Status of scripts/qmp/ (was: [PATCH 0/3] scripts/qmp/qom-fuse: Scrape off the bitrot)

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 23/07/2020 16.27, Markus Armbruster wrote:
>> Markus Armbruster (3):
>>   scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol
>>   scripts/qmp/qom-fuse: Port to current Python module fuse
>>   scripts/qmp/qom-fuse: Fix getattr(), read() for files in /
>
> Could it be added to a CI pipeline, so that it does not bitrot again?

Should it be?

Thread hijack!

What's the status of scripts/qmp/?

The directory is covered by MAINTAINERS section QMP, with status
"Supported".  Status is a *lie* for these scripts.  I inherited them
with the rest of QMP.  I have no use for them, except I occasionally use
qom-fuse for QOM spelunking, mostly because our monitor commands are so
unwieldy compared to a filesystem.  I barely looked at them in the 5+
years of my service as QMP maintainer.  Actual status is "Odd fixes".

Does this stuff have any business in /scripts/?

Nothing in scripts/qmp/ should be shipped.

scripts/qmp/qemu-ga-client doesn't even belong there.  Michael, is it of
any use today?

I know scripts/qmp/qmp-shell has a few friends among developers.  I
regard it as a failed attempt at making QMP easier for humans, and have
zero interest in it.

scripts/qmp/qmp looks like an attempt at making QMP easier for shell
scripts.  I'm not aware of actual use, and have zero interest in it.

scripts/qmp/qom-{get,list,set} look like an attempt at making QOM easier
for shell scripts.  I'm not aware of actual use, and have zero interest
in it.  Heck, I can't even figure out how to use qom-get (I just spend
at least 30s trying).

scripts/qmp/qom-tree feels redundant with scripts/qmp/qom-fuse.  I just
ran it for the first time just to come to this judgement.

I believe contrib/ would be a better home for all of them.

I feel like moving the directory there and leaving it *uncovered* in
MAINTAINERS.  If a volunteer steps forward, we can add a suitable
section.

Opinions?

Re: Status of scripts/qmp/ (was: [PATCH 0/3] scripts/qmp/qom-fuse: Scrape off the bitrot)

[Thomas Huth]

On 24/07/2020 09.33, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 23/07/2020 16.27, Markus Armbruster wrote:
>>> Markus Armbruster (3):
>>>   scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol
>>>   scripts/qmp/qom-fuse: Port to current Python module fuse
>>>   scripts/qmp/qom-fuse: Fix getattr(), read() for files in /
>>
>> Could it be added to a CI pipeline, so that it does not bitrot again?
> 
> Should it be?
> 
> Thread hijack!
> 
> What's the status of scripts/qmp/?
> 
> The directory is covered by MAINTAINERS section QMP, with status
> "Supported".  Status is a *lie* for these scripts.  I inherited them
> with the rest of QMP.  I have no use for them, except I occasionally use
> qom-fuse for QOM spelunking, mostly because our monitor commands are so
> unwieldy compared to a filesystem.  I barely looked at them in the 5+
> years of my service as QMP maintainer.  Actual status is "Odd fixes".
> 
> Does this stuff have any business in /scripts/?
> 
> Nothing in scripts/qmp/ should be shipped.
> 
> scripts/qmp/qemu-ga-client doesn't even belong there.  Michael, is it of
> any use today?
> 
> I know scripts/qmp/qmp-shell has a few friends among developers.  I
> regard it as a failed attempt at making QMP easier for humans, and have
> zero interest in it.
> 
> scripts/qmp/qmp looks like an attempt at making QMP easier for shell
> scripts.  I'm not aware of actual use, and have zero interest in it.
> 
> scripts/qmp/qom-{get,list,set} look like an attempt at making QOM easier
> for shell scripts.  I'm not aware of actual use, and have zero interest
> in it.  Heck, I can't even figure out how to use qom-get (I just spend
> at least 30s trying).

According to the original commit (235fe3bfd46b1104575b540d0bc), it seems
like these were using for manual testing. If in all those years nobody
ever tried to integrate them into our "make check" test suite, I guess
they are just dead code.

> scripts/qmp/qom-tree feels redundant with scripts/qmp/qom-fuse.  I just
> ran it for the first time just to come to this judgement.
> 
> I believe contrib/ would be a better home for all of them.
> 
> I feel like moving the directory there and leaving it *uncovered* in
> MAINTAINERS.  If a volunteer steps forward, we can add a suitable
> section.
> 
> Opinions?

I'd suggest to remove the "test tools" from commit 235fe3bfd46b1104575b5
since apparently nobody ever cared to integrate them into the test suite.

For the other scripts that are still used occasionally, I'd leave them
in their current location. If you don't want to maintain them, remove
them from your section in MAINTAINERS and add a new "QMP scripts"
section where you can mark the scripts/qmp folder as orphan.

 Thomas

Re: [PATCH 0/3] scripts/qmp/qom-fuse: Scrape off the bitrot

[John Snow]

On 7/23/20 11:21 AM, Thomas Huth wrote:
> On 23/07/2020 16.27, Markus Armbruster wrote:
>> Markus Armbruster (3):
>>    scripts/qmp/qom-fuse: Unbreak import of QEMUMonitorProtocol
>>    scripts/qmp/qom-fuse: Port to current Python module fuse
>>    scripts/qmp/qom-fuse: Fix getattr(), read() for files in /
> 
> Could it be added to a CI pipeline, so that it does not bitrot again?
> 
>   Thomas
> 

Honestly, I'm working on it, but I could use some help getting the 
python directory into shape so I can do it.

I am trying to add pylint/mypy/flake8 tests to python/qemu to prevent 
that code from bitrot, but the review/discussion there didn't go anywhere.

Once there is a solid regime in place for python/qemu/ that is part of 
CI, I can work on moving more scripts and tooling there to start 
including those as part of the CI regime to prevent rot.

--js