On Mon, Sep 14, 2009 at 06:50:14PM +0100, Matthew Booth wrote:
On 14/09/09 13:56, 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?
* Don't just comment out xread: remove it. These pile up over time and make the code unreadable.
* 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.
* guestfs__send and send_file_chunk both leak msg_out in the non-error case.
I'll continue until I have to go.
As discussed on IRC:
In recv_from_daemon, why not allocate buf_rtn fully once its size is known?
recv_file_data leak buf if return == -1 (e.g. second read in recv_from_daemon fails)
receive_file_data leak buf if return == -1
Apart from that, I think this patch is the right direction, it clearly works and should get as much testing as possible.
ACK.
Thanks - I fixed the above issues, and ran another round of testing just to make sure, and after that it still passed, so I will now push it. 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/