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

[Anthony Liguori <anthony@codemonkey.ws>]

alex@csgraf.de wrote:
> From: Alexander Graf <agraf@suse.de>
>
> 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.

> 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 <agraf@suse.de>
> + *
> + * 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

Also, make sure DEBUG_CURL is off by default (you have debug enabled in 
this patch).
> +#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?

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

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

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

Regards,

Anthony Liguori