6:10 a.m.
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/
6:44 a.m.
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
6:51 a.m.
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
7:18 a.m.
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
7:37 a.m.
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
7:46 a.m.
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:
http://libguestfs.org/FAQ.html#distros
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
8:06 a.m.
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:
>
> http://libguestfs.org/FAQ.html#distros
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
6:45 a.m.
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
6:54 a.m.
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
7:56 a.m.
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
8:45 a.m.
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/
12:50 p.m.
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
1:11 p.m.
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/
5548
days inactive
5548
days old
12 comments
2 participants
participants (2)
-
Matthew Booth -
Richard W.M. Jones