Nir Soffer
2018-Oct-15 15:43 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Mon, Oct 15, 2018 at 6:21 PM Richard W.M. Jones <rjones@redhat.com> wrote:> On Tue, Oct 09, 2018 at 02:28:10PM +0300, Nir Soffer wrote: > > > +# Create a background thread running a web server which is > > > +# simulating the imageio server. > > > > > > > This functionality should be separated from the fake SDK module, since > it is > > not part of the SDK, and may be replaced by real imageio server later. > > Well possibly, but it was very convenient to put it in the class here, > and this test is meant for running completely standalone without any > other service available. > > > > +class RequestHandler(BaseHTTPRequestHandler): > > > > > > > This request handler is using HTTP/1.0, and will close the connection > after > > every request. This is not a good implementation of the imageio server, > and > > also > > hides bugs in this code. > > > > Should be fixed by adding: > > > > protocol_version = "HTTP/1.1" > > I tried the attached patch, but for some reason it just hangs. >The issue is probably missing "content-length: 0" header in the response. If we don't close the connection and don't send content-length the client cannot do much but wait :-)> > + def do_OPTIONS(self): > > > > + self.send_response(200) > > > + self.send_header("Content-type", "application/json; > > > charset=UTF-8") > > > + self.end_headers() > > > + # Advertize only zero support. > > > + self.wfile.write(b'''{ "features": [ "zero" ] }''') > > > + > > > + # eg. zero request. Just ignore it. > > > + def do_PATCH(self): > > > > > > > This must read content-length bytes from self.rfile. > > > > > > > + self.send_response(200) > > > + self.end_headers() > > > + > > > + # Flush request. Ignore it. > > > + def do_PUT(self): > > > > > > > This must read content-length bytes from self.rfile. > > > > Both bugs are hidden by the fact that this server close the connection at > > the end > > of the request. > > Also this is covered by the attached patch. > > > > + self.send_response(200) > > > + self.end_headers() > > > + > > > +def server(): > > > + server_address = ("", imageio_port) > > > + httpd = HTTPServer(server_address, RequestHandler) > > > + httpd.serve_forever() > > > > > > > Using HTTP instead of HTTPS is not a good idea. We are not testing > > the same code on the client side. > > > > It is easy to run HTTPS server using pre-created certificate, if we > > disable certificate verification on the client side > > (e.g. --insecure). > > This one doesn't seem to be as simple to fix. However point taken, so > I added a note in the code. > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat > http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-builder quickly builds VMs from scratch > http://libguestfs.org/virt-builder.1.html >
Richard W.M. Jones
2018-Oct-15 16:13 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Mon, Oct 15, 2018 at 06:43:10PM +0300, Nir Soffer wrote:> On Mon, Oct 15, 2018 at 6:21 PM Richard W.M. Jones <rjones@redhat.com> > wrote: > > > On Tue, Oct 09, 2018 at 02:28:10PM +0300, Nir Soffer wrote: > > > > +# Create a background thread running a web server which is > > > > +# simulating the imageio server. > > > > > > > > > > This functionality should be separated from the fake SDK module, since > > it is > > > not part of the SDK, and may be replaced by real imageio server later. > > > > Well possibly, but it was very convenient to put it in the class here, > > and this test is meant for running completely standalone without any > > other service available. > > > > > > +class RequestHandler(BaseHTTPRequestHandler): > > > > > > > > > > This request handler is using HTTP/1.0, and will close the connection > > after > > > every request. This is not a good implementation of the imageio server, > > and > > > also > > > hides bugs in this code. > > > > > > Should be fixed by adding: > > > > > > protocol_version = "HTTP/1.1" > > > > I tried the attached patch, but for some reason it just hangs. > > > > The issue is probably missing "content-length: 0" header in the > response. If we don't close the connection and don't send > content-length the client cannot do much but wait :-)Ah yes, it was missing from the OPTIONS method. The attached patch does work. What do you think? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Nir Soffer
2018-Oct-16 07:32 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Mon, Oct 15, 2018 at 7:13 PM Richard W.M. Jones <rjones@redhat.com> wrote:> On Mon, Oct 15, 2018 at 06:43:10PM +0300, Nir Soffer wrote: > > On Mon, Oct 15, 2018 at 6:21 PM Richard W.M. Jones <rjones@redhat.com> > > wrote: > > > > > On Tue, Oct 09, 2018 at 02:28:10PM +0300, Nir Soffer wrote: > > > > > +# Create a background thread running a web server which is > > > > > +# simulating the imageio server. > > > > > > > > > > > > > This functionality should be separated from the fake SDK module, > since > > > it is > > > > not part of the SDK, and may be replaced by real imageio server > later. > > > > > > Well possibly, but it was very convenient to put it in the class here, > > > and this test is meant for running completely standalone without any > > > other service available. > > > > > > > > +class RequestHandler(BaseHTTPRequestHandler): > > > > > > > > > > > > > This request handler is using HTTP/1.0, and will close the connection > > > after > > > > every request. This is not a good implementation of the imageio > server, > > > and > > > > also > > > > hides bugs in this code. > > > > > > > > Should be fixed by adding: > > > > > > > > protocol_version = "HTTP/1.1" > > > > > > I tried the attached patch, but for some reason it just hangs. > > > > > > > The issue is probably missing "content-length: 0" header in the > > response. If we don't close the connection and don't send > > content-length the client cannot do much but wait :-) > > Ah yes, it was missing from the OPTIONS method. > The attached patch does work. What do you think? >Looks good Nir
Richard W.M. Jones
2018-Nov-20 17:04 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Mon, Oct 15, 2018 at 05:13:31PM +0100, Richard W.M. Jones wrote:> On Mon, Oct 15, 2018 at 06:43:10PM +0300, Nir Soffer wrote: > > On Mon, Oct 15, 2018 at 6:21 PM Richard W.M. Jones <rjones@redhat.com> > > wrote: > > > > > On Tue, Oct 09, 2018 at 02:28:10PM +0300, Nir Soffer wrote: > > > > > +# Create a background thread running a web server which is > > > > > +# simulating the imageio server. > > > > > > > > > > > > > This functionality should be separated from the fake SDK module, since > > > it is > > > > not part of the SDK, and may be replaced by real imageio server later. > > > > > > Well possibly, but it was very convenient to put it in the class here, > > > and this test is meant for running completely standalone without any > > > other service available. > > > > > > > > +class RequestHandler(BaseHTTPRequestHandler): > > > > > > > > > > > > > This request handler is using HTTP/1.0, and will close the connection > > > after > > > > every request. This is not a good implementation of the imageio server, > > > and > > > > also > > > > hides bugs in this code. > > > > > > > > Should be fixed by adding: > > > > > > > > protocol_version = "HTTP/1.1" > > > > > > I tried the attached patch, but for some reason it just hangs. > > > > > > > The issue is probably missing "content-length: 0" header in the > > response. If we don't close the connection and don't send > > content-length the client cannot do much but wait :-) > > Ah yes, it was missing from the OPTIONS method. > The attached patch does work. What do you think? > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-builder quickly builds VMs from scratch > http://libguestfs.org/virt-builder.1.html> >From 6eb473a7e67de82bf08b97f2610ee3364071c741 Mon Sep 17 00:00:00 2001 > From: "Richard W.M. Jones" <rjones@redhat.com> > Date: Mon, 15 Oct 2018 16:07:52 +0100 > Subject: [PATCH] v2v: -o rhv-upload: Test using HTTP/1.1 protocol. > > Fixes commit b54b58c44e3f1f54a05117c758eaa21d9f1085f0. > > Thanks: Nir Soffer. > --- > .../ovirtsdk4/__init__.py | 28 ++++++++++++++++--- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > index b5f739471..84b9d56aa 100644 > --- a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > +++ b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > @@ -116,24 +116,44 @@ from http.server import HTTPServer, BaseHTTPRequestHandler > import threading > > class RequestHandler(BaseHTTPRequestHandler): > + protocol_version = 'HTTP/1.1' > + > def do_OPTIONS(self): > - self.send_response(200) > - self.send_header("Content-type", "application/json; charset=UTF-8") > - self.end_headers() > + self.discard_request() > + > # Advertize only zero support. > - self.wfile.write(b'''{ "features": [ "zero" ] }''') > + content = b'''{ "features": [ "zero" ] }''' > + length = len(content) > + > + self.send_response(200) > + self.send_header("Content-type", "application/json; charset=UTF-8") > + self.send_header("Content-Length", length) > + self.end_headers() > + self.wfile.write(content) > > # eg. zero request. Just ignore it. > def do_PATCH(self): > + self.discard_request() > self.send_response(200) > + self.send_header("Content-Length", "0") > self.end_headers() > > # Flush request. Ignore it. > def do_PUT(self): > + self.discard_request() > self.send_response(200) > + self.send_header("Content-Length", "0") > self.end_headers() > > + def discard_request(self): > + length = self.headers['Content-Length'] > + if length: > + length = int(length) > + content = self.rfile.read(length)This commit went upstream: https://github.com/libguestfs/libguestfs/commit/bfd9bee9fc7a91fdca0ca051deca062ff29d1f01 However it fails on RHEL 7.6 with: Traceback (most recent call last): File "/usr/lib64/python2.7/SocketServer.py", line 295, in _handle_request_nobl ock self.process_request(request, client_address) File "/usr/lib64/python2.7/SocketServer.py", line 321, in process_request self.finish_request(request, client_address) File "/usr/lib64/python2.7/SocketServer.py", line 334, in finish_request self.RequestHandlerClass(request, client_address, self) File "/usr/lib64/python2.7/SocketServer.py", line 649, in __init__ self.handle() File "/usr/lib64/python2.7/BaseHTTPServer.py", line 340, in handle self.handle_one_request() File "/usr/lib64/python2.7/BaseHTTPServer.py", line 328, in handle_one_request method() File "/home/rjones/d/libguestfs-rhel-7.6-lp/v2v/test-v2v-o-rhv-upload-module/o virtsdk4/__init__.py", line 123, in do_OPTIONS self.discard_request() File "/home/rjones/d/libguestfs-rhel-7.6-lp/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py", line 150, in discard_request length = self.headers['Content-Length'] File "/usr/lib64/python2.7/rfc822.py", line 388, in __getitem__ return self.dict[name.lower()] KeyError: 'content-length' I guess that Python won't let us do: length = self.headers['Content-Length'] if length: How about the attached fix? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/