[Qemu-devel] Re: [PATCH] Add HTTP protocol using curl v4

[Alexander Graf]

On 07.05.2009, at 15:29, Anthony Liguori wrote:

> address@hidden wrote:
> > From: Alexander Graf <address@hidden>
> >
> > Currently Qemu can read from posix I/O and NBD. This patch adds a
> > third protocol to the game: HTTP.
> >
> > In certain situations it can be useful to access HTTP data directly,
> > for example if you want to try out an http provided OS image, but
> > don't know if you want to download it yet.
> >
> > Using this patch you can now try it on on the fly. Just use it like:
> >
> > qemu -cdrom http://host/path/my.iso
> >
> > In order to not reinvent the wheel, this patch uses libcurl.
> >
> > v2 changes:
> >
> > - fix the segfault (yay!)
> > - implement AIO
> >
> > v3 changes:
> >
> > - remove synchronous API
> > - implement caching
> >
> > v4 changes:
> >
> > - enable other protocols (HTTPS, FTP, FTPS, TFTP, SFTP, SCP)
> >  (I only tested FTP so far, but they _should_ work)
>
> So now all of the references to http are a big fat lie? :-)
>
> Perhaps we should s/http/curl/g.

Sounds like a good idea, yeah.

> > diff --git a/block-http.c b/block-http.c
> > new file mode 100644
> > index 0000000..3045cbb
> > --- /dev/null
> > +++ b/block-http.c
> > @@ -0,0 +1,534 @@
> > +/*
> > + * QEMU Block driver for HTTP images
> > + *
> > + * Copyright (c) 2009 Alexander Graf <address@hidden>
> > + *
> > + * Permission is hereby granted, free of charge, to any person  
> > obtaining a copy
> > + * of this software and associated documentation files (the  
> > "Software"), to deal
> > + * in the Software without restriction, including without  
> > limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense,  
> > and/or sell
> > + * copies of the Software, and to permit persons to whom the  
> > Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be  
> > included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,  
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF  
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO  
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,  
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,  
> > ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER  
> > DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +#include "qemu-common.h"
> > +#include "block_int.h"
> > +#include <curl/curl.h>
> > +
> > +#define DEBUG
> > +#define DEBUG_VERBOSE
> > +
> > +#ifdef DEBUG
> > +#define printd printf
> > +#else
> > +#define printd(x, ...)
> > +#endif
>
> I'd prefer dprintf().  Also, the standard way to do this is:
>
> //#define DEBUG_CURL
>
> #ifdef DEBUG_CURL
> #define dprintf(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while  
> (0)
> #else
> #define dprintf(fmt, ...) do { } while (0)
> #endif

Ok.

> Also, make sure DEBUG_CURL is off by default (you have debug enabled  
> in this patch).

Ugh. Right - I checked on every former version but apparently forgot  
on this one.

> > +#define HTTP_NUM_STATES 8
> > +
> > +static size_t http_size_cb(void *ptr, size_t size, size_t nmemb,  
> > void *opaque)
> > +{
> > +    HTTPState *s = ((HTTPState*)opaque);
> > +    size_t realsize = size * nmemb;
> > +    uint64_t fsize;
> > +
> > +    if(sscanf(ptr, "Content-Length: %lld", &fsize) == 1)
> > +        s->s->len = fsize;
>
> How does this work with other protocols?  Certainly, there isn't a  
> Content-Length header with scp?

Curl emulates the Content-Length write - at least it does so with FTP.  
As all curl versions I have lying around don't have USE_LIBSSH2  
enabled, I couldn't verify that the scp backend does the same, but I  
suppose it does.

> > +static int http_find_buf(BDRVHTTPState *s, long long start, long  
> > long len,
> > +                         HTTPAIOCB *acb)
> > +{
> > +    int i;
> > +    long long end = start + len;
>
> size_t is the right type to represent lengths/offsets in a memory  
> buffer.

Ok.

> > +static void http_close(BlockDriverState *bs)
> > +{
> > +    BDRVHTTPState *s = bs->opaque;
> > +    int i;
>
> You strdup() url but don't free it.  I'd suggest building qemu-io  
> and running it under valgrind to verify that you aren't leaking  
> other places too.

The strdup() should be the only one, but I can verify.

> > +BlockDriver bdrv_http = {
> > +    .format_name       = "http",
> > +    .protocol_name     = "http",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
> > +
> > +BlockDriver bdrv_https = {
> > +    .format_name       = "https",
> > +    .protocol_name     = "https",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
> > +
> > +BlockDriver bdrv_ftp = {
> > +    .format_name       = "ftp",
> > +    .protocol_name     = "ftp",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
> > +
> > +BlockDriver bdrv_ftps = {
> > +    .format_name       = "ftps",
> > +    .protocol_name     = "ftps",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
> > +
> > +BlockDriver bdrv_sftp = {
> > +    .format_name       = "sftp",
> > +    .protocol_name     = "sftp",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
> > +
> > +BlockDriver bdrv_scp = {
> > +    .format_name       = "scp",
> > +    .protocol_name     = "scp",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
> > +
> > +BlockDriver bdrv_tftp = {
> > +    .format_name       = "tftp",
> > +    .protocol_name     = "tftp",
> > +
> > +    .instance_size     = sizeof(BDRVHTTPState),
> > +    .bdrv_open         = http_open,
> > +    .bdrv_close                = http_close,
> > +    .bdrv_getlength    = http_getlength,
> > +
> > +    .aiocb_size                = sizeof(HTTPAIOCB),
> > +    .bdrv_aio_readv    = http_aio_readv,
> > +    .bdrv_aio_cancel    = http_aio_cancel,
> > +};
>
> It's rather slick to get all of these for free.

Want me to enable ldap too? :-)

Alex