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

[Eduardo Habkost <ehabkost@redhat.com>]

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