[Libguestfs] [PATCH REBASED] Remove main loop

[Richard W.M. Jones]

On Mon, Sep 14, 2009 at 01:56:35PM +0100, Matthew Booth wrote:
> On 14/09/09 12:10, Richard W.M. Jones wrote:
>> This is the only patch I currently have outstanding.  No changes from
>> the previous posting, except I rebased it against the head of git.
>
> I'm running out of time, so I'm going to dump what I've got:
>
> * I already moaned about gotos.
>
> * Why does the patch change the set_trace test in generator.ml?

Should have explained - this was broken.  If we enable set_trace in
the test, then it means all tests that follow it have tracing enabled.
This is not a show-stopper or anything like that, but it is
undesirable.  It should be a separate patch, but I noticed it and
slipped the small change in.

> * Don't just comment out xread: remove it. These pile up over time and  
> make the code unreadable.

Done.

> * Don't put a usleep() in read_log_message_or_eog. I understand your  
> argument, but in this case the cure is worse than the disease. If you  
> think about it, the patch back into the main loop and back here is  
> actually very short, and very cheap. As well as being ugly as sin, this  
> usleep is only going to slow it down.

strace reveals this makes a difference.  Without it we end up with:

  read (of 1-16 bytes, usually 1-2 bytes)
  select
  read (of 1-16 bytes, usually 1-2 bytes)
  select
  [...]

With it we get much larger reads and we don't reenter the select.

_On the other hand_ the size of the sleep is difficult to judge.  If
it's any smaller than the current value then qemu doesn't have enough
time to coalesce the serial port messages.  This probably also means
that it is sensitive to the particular details of my test machine.

> * guestfs__send and send_file_chunk both leak msg_out in the non-error case.

Ugh, will fix that one.

> I'll continue until I have to go.

Thanks, good luck.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/