Re: [Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Michael Roth]

On 11/09/2010 04:45 AM, Amit Shah wrote:
> On (Fri) Nov 05 2010 [08:32:30], Adam Litke wrote:
> > On Thu, 2010-11-04 at 08:57 -0500, Michael Roth wrote:
> > > > This resembles vl.c's main_loop_wait() but I can see why you might want
> > > > your own.  There is opportunity for sharing the select logic and ioh
> > > > callbacks but I think that could be addressed later.
> > >
> > > Yup these are all basically copy/pastes from vl.c. It feels a bit dirty,
> > > but I modeled things after the other qemu tools like qemu-nbd/qemu-io,
> > > which don't link against vl.c (and adding a target for tools to do so
> > > looks like it'd be a bit hairy since vl.c touches basically everything).
> >
> > > It might still make sense to share things like structs...but the ones
> > > I'm stealing here are specific to reproducing the main_loop_wait logic.
> > > So I guess the real question is whether main_loop_wait and friends make
> > > sense to expose as a utility function of some sort, and virtproxy seems
> > > to be the only use case so far.
> >
> > You make a fair point about following precedent, but the thought of
> > dual-maintaining code into the future is not that appealing.  I guess we
> > could benefit from other voices on this topic.
>
> I agree we should share the code -- I have some patches for qemu
> chardevs to behave reasonably when buffers are full (so we don't see
> guest hangs).  You'll benefit as soon as that work enters git.
>
>                 Amit

Thanks. I've been giving this a good look, but I'm still not sure I 
agree on how much code/future work we'd really be saving.

To be clear I basically re-implement the following:

qemu-char.c:send_all()
vl.c:qemu_set_fd_handler2()

qemu-vp.c has a main_loop_wait() that closely mirrors the one in vl.c, 
but I don't think that could be shared, or broken out into shareable 
bits in any meaningful way.

So the only thing in qemu-char.c I'm actually using/re-implementing is 
qemu-char.c:send_all(), and I think it'd make more sense to move this 
function out of qemu-char.c rather than fixing up the qemu tool targets 
(qemu-nbd/qemu-img/etc) to link against qemu-char.c, none of which 
currently seem to (nor does qemu-vp). Maybe qemu-sockets.c?

vl.c:qemu_set_fd_handler*() is a bit more involved, it touches state 
information specific to vl.c (vl.c:io_handlers list mainly, which is 
static, and vl.c:main_loop_wait loops through and modifies it), and 
currently it's noop'd in qemu-tool.c for standalone binaries.

Am I right in thinking the most logical approach is to move it out of 
qemu-tool.c/vl.c and into some common obj, along with vl.c:io_handlers 
and friends?

The major downside there is that to implement the i/o loop in qemu-vp.c, 
or any tool implementing an i/o loop modeled after main_loop_wait() and 
using qemu_set_fd_handler(), I'd need to be able to access/modify the 
io_handlers list, which means it'd need to be global rather than static 
as it is in vl.c.

I'm not sure how well that'd go over since it effectively allows us to 
bypass the qemu_set_fd_handler* interfaces and muck with it directly. 
And I'm not sure there's a reasonable way to share this code without 
making that tradeoff, or modifying these interfaces...which would touch 
quite a bit of code. Or maybe this isn't that big a concern, in which 
case I'd be willing to take a whack putting together a patch.

-Mike