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.

> > +    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