[Qemu-devel] [PATCH 3/4] scripts/qemu: add render_block_graph method for QEMUMachine

Vladimir Sementsov-Ogievskiy posted 4 patches 7 years, 2 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 3/4] scripts/qemu: add render_block_graph method for QEMUMachine
Posted by Vladimir Sementsov-Ogievskiy 7 years, 2 months ago
Render block nodes graph with help of graphviz

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/qemu.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index f099ce7278..cff562c713 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -460,3 +460,56 @@ class QEMUMachine(object):
                                                  socket.SOCK_STREAM)
             self._console_socket.connect(self._console_address)
         return self._console_socket
+
+    def render_block_graph(self, filename):
+        '''
+        Render graph in text (dot) representation into "filename" and graphical
+        representation into pdf file "filename".pdf
+        '''
+
+        try:
+            from graphviz import Digraph
+        except ImportError:
+            print "Can't import graphviz. Please run 'pip install graphviz'"
+            return
+
+        nodes = self.qmp('query-named-block-nodes')['return']
+        edges = self.qmp('x-query-block-nodes-relations')['return']
+        node_names = []
+
+        graph = Digraph(comment='Block Nodes Graph')
+        graph.node('permission symbols:\l'
+                   ' w - Write\l'
+                   ' r - consistent-Read\l'
+                   ' u - write - Unchanged\l'
+                   ' g - Graph-mod\l'
+                   ' s - reSize\l'
+                   'edge label scheme:\l'
+                   ' <child type>\l'
+                   ' <perm>\l'
+                   ' <shared_perm>\l', shape='none')
+
+        def perm(arr):
+            s = 'w' if 'write' in arr else '_'
+            s += 'r' if 'consistent-read' in arr else '_'
+            s += 'u' if 'write-unchanged' in arr else '_'
+            s += 'g' if 'graph-mod' in arr else '_'
+            s += 's' if 'resize' in arr else '_'
+            return s
+
+        for n in nodes:
+            node_names.append(n['node-name'])
+            label = n['node-name'] + ' [' + n['drv'] + ']'
+            if n['drv'] == 'file':
+                label = '<%s<br/>%s>' % (label, os.path.basename(n['file']))
+            graph.node(n['node-name'], label)
+
+        for r in edges:
+            if r['parent'] not in node_names:
+                graph.node(r['parent'], shape='box')
+
+            label = '%s\l%s\l%s\l' % (r['name'], perm(r['perm']),
+                                      perm(r['shared-perm']))
+            graph.edge(r['parent'], r['child'], label=label)
+
+        graph.render(filename)
-- 
2.11.1


Re: [Qemu-devel] [PATCH 3/4] scripts/qemu: add render_block_graph method for QEMUMachine
Posted by Eduardo Habkost 7 years, 2 months ago
On Thu, Aug 16, 2018 at 08:20:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Render block nodes graph with help of graphviz
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks for your patch.  Comments below:

> ---
>  scripts/qemu.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index f099ce7278..cff562c713 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -460,3 +460,56 @@ class QEMUMachine(object):
>                                                   socket.SOCK_STREAM)
>              self._console_socket.connect(self._console_address)
>          return self._console_socket
> +
> +    def render_block_graph(self, filename):
> +        '''
> +        Render graph in text (dot) representation into "filename" and graphical
> +        representation into pdf file "filename".pdf
> +        '''
> +
> +        try:
> +            from graphviz import Digraph

I'm not convinced that this belongs to qemu.py, if most code
using the qemu.py module will never use it.  Do you see any
problem in moving this code to a scripts/render_block_graph.py
script?

> +        except ImportError:
> +            print "Can't import graphviz. Please run 'pip install graphviz'"

If you really want to make this part of qemu.py, I suggest
raising an appropriate exception instead of printing to stdout
directly.

(But just moving this code to a separate script would probably be
simpler)

Also, keep in mind that this module needs to work with both
Python 3 and Python 2, so you need to use print("message ...")
with parenthesis.


> +            return
> +
> +        nodes = self.qmp('query-named-block-nodes')['return']
> +        edges = self.qmp('x-query-block-nodes-relations')['return']

I suggest you use .command() instead of .qmp().  It handles
['return'] and ['error'] for you.


> +        node_names = []
> +
> +        graph = Digraph(comment='Block Nodes Graph')
> +        graph.node('permission symbols:\l'
> +                   ' w - Write\l'
> +                   ' r - consistent-Read\l'
> +                   ' u - write - Unchanged\l'
> +                   ' g - Graph-mod\l'
> +                   ' s - reSize\l'
> +                   'edge label scheme:\l'
> +                   ' <child type>\l'
> +                   ' <perm>\l'
> +                   ' <shared_perm>\l', shape='none')
> +
> +        def perm(arr):
> +            s = 'w' if 'write' in arr else '_'
> +            s += 'r' if 'consistent-read' in arr else '_'
> +            s += 'u' if 'write-unchanged' in arr else '_'
> +            s += 'g' if 'graph-mod' in arr else '_'
> +            s += 's' if 'resize' in arr else '_'
> +            return s
> +
> +        for n in nodes:
> +            node_names.append(n['node-name'])
> +            label = n['node-name'] + ' [' + n['drv'] + ']'
> +            if n['drv'] == 'file':
> +                label = '<%s<br/>%s>' % (label, os.path.basename(n['file']))
> +            graph.node(n['node-name'], label)
> +
> +        for r in edges:
> +            if r['parent'] not in node_names:
> +                graph.node(r['parent'], shape='box')
> +
> +            label = '%s\l%s\l%s\l' % (r['name'], perm(r['perm']),
> +                                      perm(r['shared-perm']))
> +            graph.edge(r['parent'], r['child'], label=label)
> +
> +        graph.render(filename)

The rest of the code looks OK to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/4] scripts/qemu: add render_block_graph method for QEMUMachine
Posted by Vladimir Sementsov-Ogievskiy 7 years, 2 months ago
17.08.2018 04:54, Eduardo Habkost wrote:
> On Thu, Aug 16, 2018 at 08:20:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Render block nodes graph with help of graphviz
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Thanks for your patch.  Comments below:
>
>> ---
>>   scripts/qemu.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index f099ce7278..cff562c713 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -460,3 +460,56 @@ class QEMUMachine(object):
>>                                                    socket.SOCK_STREAM)
>>               self._console_socket.connect(self._console_address)
>>           return self._console_socket
>> +
>> +    def render_block_graph(self, filename):
>> +        '''
>> +        Render graph in text (dot) representation into "filename" and graphical
>> +        representation into pdf file "filename".pdf
>> +        '''
>> +
>> +        try:
>> +            from graphviz import Digraph
> I'm not convinced that this belongs to qemu.py, if most code
> using the qemu.py module will never use it.  Do you see any
> problem in moving this code to a scripts/render_block_graph.py
> script?

Not a problem, I can do it.. Just one thought:
Isn't it better to keep all function doing something with one vm as 
methods, not separate functions?

>
>> +        except ImportError:
>> +            print "Can't import graphviz. Please run 'pip install graphviz'"
> If you really want to make this part of qemu.py, I suggest
> raising an appropriate exception instead of printing to stdout
> directly.

Ok

> (But just moving this code to a separate script would probably be
> simpler)
>
> Also, keep in mind that this module needs to work with both
> Python 3 and Python 2, so you need to use print("message ...")
> with parenthesis.
>
>
>> +            return
>> +
>> +        nodes = self.qmp('query-named-block-nodes')['return']
>> +        edges = self.qmp('x-query-block-nodes-relations')['return']
> I suggest you use .command() instead of .qmp().  It handles
> ['return'] and ['error'] for you.

Cool, thanks

>
>
>> +        node_names = []
>> +
>> +        graph = Digraph(comment='Block Nodes Graph')
>> +        graph.node('permission symbols:\l'
>> +                   ' w - Write\l'
>> +                   ' r - consistent-Read\l'
>> +                   ' u - write - Unchanged\l'
>> +                   ' g - Graph-mod\l'
>> +                   ' s - reSize\l'
>> +                   'edge label scheme:\l'
>> +                   ' <child type>\l'
>> +                   ' <perm>\l'
>> +                   ' <shared_perm>\l', shape='none')
>> +
>> +        def perm(arr):
>> +            s = 'w' if 'write' in arr else '_'
>> +            s += 'r' if 'consistent-read' in arr else '_'
>> +            s += 'u' if 'write-unchanged' in arr else '_'
>> +            s += 'g' if 'graph-mod' in arr else '_'
>> +            s += 's' if 'resize' in arr else '_'
>> +            return s
>> +
>> +        for n in nodes:
>> +            node_names.append(n['node-name'])
>> +            label = n['node-name'] + ' [' + n['drv'] + ']'
>> +            if n['drv'] == 'file':
>> +                label = '<%s<br/>%s>' % (label, os.path.basename(n['file']))
>> +            graph.node(n['node-name'], label)
>> +
>> +        for r in edges:
>> +            if r['parent'] not in node_names:
>> +                graph.node(r['parent'], shape='box')
>> +
>> +            label = '%s\l%s\l%s\l' % (r['name'], perm(r['perm']),
>> +                                      perm(r['shared-perm']))
>> +            graph.edge(r['parent'], r['child'], label=label)
>> +
>> +        graph.render(filename)
> The rest of the code looks OK to me.
>


-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH 3/4] scripts/qemu: add render_block_graph method for QEMUMachine
Posted by Eduardo Habkost 7 years, 2 months ago
On Fri, Aug 17, 2018 at 01:08:42PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 17.08.2018 04:54, Eduardo Habkost wrote:
> > On Thu, Aug 16, 2018 at 08:20:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Render block nodes graph with help of graphviz
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > Thanks for your patch.  Comments below:
> > 
> > > ---
> > >   scripts/qemu.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 53 insertions(+)
> > > 
> > > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > > index f099ce7278..cff562c713 100644
> > > --- a/scripts/qemu.py
> > > +++ b/scripts/qemu.py
> > > @@ -460,3 +460,56 @@ class QEMUMachine(object):
> > >                                                    socket.SOCK_STREAM)
> > >               self._console_socket.connect(self._console_address)
> > >           return self._console_socket
> > > +
> > > +    def render_block_graph(self, filename):
> > > +        '''
> > > +        Render graph in text (dot) representation into "filename" and graphical
> > > +        representation into pdf file "filename".pdf
> > > +        '''
> > > +
> > > +        try:
> > > +            from graphviz import Digraph
> > I'm not convinced that this belongs to qemu.py, if most code
> > using the qemu.py module will never use it.  Do you see any
> > problem in moving this code to a scripts/render_block_graph.py
> > script?
> 
> Not a problem, I can do it.. Just one thought:
> Isn't it better to keep all function doing something with one vm as methods,
> not separate functions?

I don't think so.  We don't need to create a new QEMUMachine
method every time we write a function that takes a QEMUMachine as
argument in our scripts.

There are cases when we're forced to create a method: when the
new code is tightly coupled to the QEMUMachine code and need
access to its private attributes.

There are cases where we probably want to place the code in
qemu.py (as a method): when we expect many users of qemu.py to
call it.

But in other cases, I don't see any problem with another
script/module having a regular function like:

  def my_special_purpose_function(vm, ...):
     ...


-- 
Eduardo