[PATCH REBASED] Remove main loop
This is the only patch I currently have outstanding. No changes from the previous posting, except I rebased it against the head of git. 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/
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 still reviewing this (and probably will be for a while). I have a high-level observation at this stage, though. Several error paths in this code (and the previous code, too) would be a whole lot simpler if LAUNCH and CANCEL were turned into real protocol messages, and receive file was terminated with ACK. This would also simplify review. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Sep 14, 2009 at 12:44:15PM +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 still reviewing this (and probably will be for a while). I have a high-level observation at this stage, though. Several error paths in this code (and the previous code, too) would be a whole lot simpler if LAUNCH and CANCEL were turned into real protocol messages, and receive file was terminated with ACK. This would also simplify review.
We can't change the protocol because it's baked into other appliances that we've shipped and people could use the wrong appliance and library version combination. (At the moment this does work). Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On 14/09/09 12:51, Richard W.M. Jones wrote:
On Mon, Sep 14, 2009 at 12:44:15PM +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 still reviewing this (and probably will be for a while). I have a high-level observation at this stage, though. Several error paths in this code (and the previous code, too) would be a whole lot simpler if LAUNCH and CANCEL were turned into real protocol messages, and receive file was terminated with ACK. This would also simplify review.
We can't change the protocol because it's baked into other appliances that we've shipped and people could use the wrong appliance and library version combination. (At the moment this does work).
Well, the appliance is an integral part of libguestfs, which is also why they're shipped together. The protocol really is an internal implementation detail, not part of the API. I don't see any problem changing the protocol. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Sep 14, 2009 at 01:18:40PM +0100, Matthew Booth wrote:
On 14/09/09 12:51, Richard W.M. Jones wrote:
On Mon, Sep 14, 2009 at 12:44:15PM +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 still reviewing this (and probably will be for a while). I have a high-level observation at this stage, though. Several error paths in this code (and the previous code, too) would be a whole lot simpler if LAUNCH and CANCEL were turned into real protocol messages, and receive file was terminated with ACK. This would also simplify review.
We can't change the protocol because it's baked into other appliances that we've shipped and people could use the wrong appliance and library version combination. (At the moment this does work).
Well, the appliance is an integral part of libguestfs, which is also why they're shipped together. The protocol really is an internal implementation detail, not part of the API. I don't see any problem changing the protocol.
In theory, but our advice for packagers (on non-Debian, non-Red Hat distributions) has been to pull the Fedora binary appliance out of the packages: http://libguestfs.org/FAQ.html#distros Also you can set LIBGUESTFS_PATH and point it to any appliance you want, and most likely it will work. Anyhow, changing the protocol itself (which is very well tested) is outside the scope of this patch. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On 14/09/09 13:37, Richard W.M. Jones wrote:
On Mon, Sep 14, 2009 at 01:18:40PM +0100, Matthew Booth wrote:
On 14/09/09 12:51, Richard W.M. Jones wrote:
On Mon, Sep 14, 2009 at 12:44:15PM +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 still reviewing this (and probably will be for a while). I have a high-level observation at this stage, though. Several error paths in this code (and the previous code, too) would be a whole lot simpler if LAUNCH and CANCEL were turned into real protocol messages, and receive file was terminated with ACK. This would also simplify review.
We can't change the protocol because it's baked into other appliances that we've shipped and people could use the wrong appliance and library version combination. (At the moment this does work).
Well, the appliance is an integral part of libguestfs, which is also why they're shipped together. The protocol really is an internal implementation detail, not part of the API. I don't see any problem changing the protocol.
In theory, but our advice for packagers (on non-Debian, non-Red Hat distributions) has been to pull the Fedora binary appliance out of the packages:
Then give it a bigger version bump and stick a release note in ;)
Also you can set LIBGUESTFS_PATH and point it to any appliance you want, and most likely it will work.
Anyhow, changing the protocol itself (which is very well tested) is outside the scope of this patch.
This patch invalidates all that testing, because it effectively changes the entire implementation of the protocol. I suggest that this is a perfect opportunity to also fix the protocol, if not in this patch, then in one to follow closely afterwards. The cancellation of file sends is certainly broken at the protocol level. Everywhere we ignore cancellation messages in the implementation is simply an indication of this. It's broken because there's no ACK at the end of a successful send, which means that the host cannot know if the file was successfully written to the guest or not unless the write fails before the end of the transmission. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Sep 14, 2009 at 01:46:22PM +0100, Matthew Booth wrote:
On 14/09/09 13:37, Richard W.M. Jones wrote:
In theory, but our advice for packagers (on non-Debian, non-Red Hat distributions) has been to pull the Fedora binary appliance out of the packages:
Then give it a bigger version bump and stick a release note in ;)
Also you can set LIBGUESTFS_PATH and point it to any appliance you want, and most likely it will work.
Anyhow, changing the protocol itself (which is very well tested) is outside the scope of this patch.
This patch invalidates all that testing, because it effectively changes the entire implementation of the protocol. I suggest that this is a perfect opportunity to also fix the protocol, if not in this patch, then in one to follow closely afterwards.
The cancellation of file sends is certainly broken at the protocol level. Everywhere we ignore cancellation messages in the implementation is simply an indication of this.
I simply don't agree. Cancellation is tested in both directions. The fact that you can't -- in the current API -- cancel an upload is a limitation of the API. One that could be fixed relatively easily. The protocol itself is simple, well-documented[1], and well-tested. This patch doesn't change the daemon side at all, and doesn't materially change the host side either (although the patch is large, the structure of the original code is preserved).
It's broken because there's no ACK at the end of a successful send, which means that the host cannot know if the file was successfully written to the guest or not unless the write fails before the end of the transmission.
This isn't true. The protocol catches this case correctly, because there _is_ an "ACK" at the end of file transfers and file failures. [1] http://libguestfs.org/guestfs.3.html#communication_protocol Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
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 forgot to mention: no more gotos, please, except for error recovery. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Sep 14, 2009 at 12:45:41PM +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 forgot to mention: no more gotos, please, except for error recovery.
I have no problem with gotos, and I think in this case they make the code much easier to understand. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
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. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
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/
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. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
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/
participants (2)
-
Matthew Booth -
Richard W.M. Jones