[Qemu-devel] [PATCH] Add HTTP protocol using curl v2

[Qemu-devel] [PATCH] Add HTTP protocol using curl v2

[alex]

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

I am still missing caching completely, so if you're accessing a WAN server
it'll be rather slow. Also, a lot of small partial accesses will clutter
the server's logs which is not ideal.

I'm open to suggestions on how to implement caching. I was thinking of a model
similar to -snapshot, but invoked from within the http backend, with the
option to write the file to a real file too, so you can use qemu as download
application while already using the image.

I haven't thought out resuming downloads for that one yet though :-/. Maybe
scanning the download file for sparse spots and force download those chunks?

Again, I'm open to suggestions :-). The patch as is is rather useful already
though as long as you stay in a LAN (or have a really fast internet connection).

Also please give me ideas on how and where to put this feature in the docs.

Signed-off-by: Alexander Graf <alex@csgraf.de>
---
 Makefile        |    6 +
 Makefile.target |    2 +-
 block-http.c    |  381 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.c         |    3 +
 block.h         |    1 +
 configure       |   25 ++++
 6 files changed, 417 insertions(+), 1 deletions(-)
 create mode 100644 block-http.c

diff --git a/Makefile b/Makefile
index 3c70068..fbfbc6e 100644
--- a/Makefile
+++ b/Makefile
@@ -77,6 +77,10 @@ endif
 BLOCK_OBJS += block-raw-posix.o
 endif
 
+ifdef CONFIG_CURL
+BLOCK_OBJS += block-http.o
+endif
+
 ######################################################################
 # libqemu_common.a: Target independent part of system emulation. The
 # long term path is to suppress *all* target specific code in case of
@@ -185,6 +189,8 @@ endif
 
 LIBS+=$(VDE_LIBS)
 
+LIBS+=$(CURL_LIBS)
+
 cocoa.o: cocoa.m
 
 keymaps.o: keymaps.c keymaps.h
diff --git a/Makefile.target b/Makefile.target
index f735105..3a9226c 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -735,7 +735,7 @@ endif
 
 vl.o: qemu-options.h
 
-$(QEMU_PROG): LIBS += $(SDL_LIBS) $(COCOA_LIBS) $(CURSES_LIBS) $(BRLAPI_LIBS) $(VDE_LIBS)
+$(QEMU_PROG): LIBS += $(SDL_LIBS) $(COCOA_LIBS) $(CURSES_LIBS) $(BRLAPI_LIBS) $(VDE_LIBS) $(CURL_LIBS)
 
 $(QEMU_PROG): $(OBJS) ../libqemu_common.a libqemu.a
 	$(LINK)
diff --git a/block-http.c b/block-http.c
new file mode 100644
index 0000000..9842b20
--- /dev/null
+++ b/block-http.c
@@ -0,0 +1,381 @@
+/*
+ * 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
+
+#ifdef CONFIG_AIO
+#define HTTP_NUM_STATES 8
+#else
+#define HTTP_NUM_STATES 1
+#endif
+
+#define SECTOR_SIZE 512
+
+struct BDRVHTTPState;
+
+#ifdef CONFIG_AIO
+typedef struct HTTPAIOCB {
+    BlockDriverAIOCB common;
+    QEMUIOVector *qiov;
+} HTTPAIOCB;
+#endif
+
+typedef struct HTTPState
+{
+    struct BDRVHTTPState *s;
+    CURL *curl;
+    uint8_t *buf;
+    int64_t buf_len;
+    char range[128];
+    char errmsg[CURL_ERROR_SIZE];
+    char in_use;
+#ifdef CONFIG_AIO
+    uint8_t *orig_buf;
+    HTTPAIOCB *acb;
+#endif
+} HTTPState;
+
+typedef struct BDRVHTTPState {
+    CURLM *multi;
+    int64_t len;
+    HTTPState states[HTTP_NUM_STATES];
+    char *url;
+} BDRVHTTPState;
+
+static void http_clean_state(HTTPState *s);
+static void http_multi_do(void *arg);
+
+#ifdef CONFIG_AIO
+static int http_sock_cb(CURL *curl, curl_socket_t fd, int action,
+                        void *s, void *sp)
+{
+    printd("CURL (AIO): Sock action %d on fd %d\n", action, fd);
+    switch (action) {
+        case CURL_POLL_IN:
+            qemu_aio_set_fd_handler(fd, http_multi_do, NULL, NULL, s);
+            break;
+        case CURL_POLL_OUT:
+            qemu_aio_set_fd_handler(fd, NULL, http_multi_do, NULL, s);
+            break;
+        case CURL_POLL_INOUT:
+            qemu_aio_set_fd_handler(fd, http_multi_do,
+                                    http_multi_do, NULL, s);
+            break;
+        case CURL_POLL_REMOVE:
+            qemu_aio_set_fd_handler(fd, NULL, NULL, NULL, NULL);
+            break;
+    }
+
+    return 0;
+}
+#endif
+
+static size_t http_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
+{
+    HTTPState *s = ((HTTPState*)opaque);
+    size_t realsize = size * nmemb;
+
+    memcpy(s->buf, ptr, realsize);
+    s->buf += realsize;
+
+    printd("CURL: Just read %lld bytes\n", (unsigned long long)realsize);
+
+    return realsize;
+}
+
+static void http_multi_do(void *arg)
+{
+#ifdef CONFIG_AIO
+    BDRVHTTPState *s = (BDRVHTTPState *)arg;
+    int running;
+    int r;
+    int msgs_in_queue;
+
+    if (!s->multi)
+        return;
+
+    do {
+        r = curl_multi_socket_all(s->multi, &running);
+    } while(r == CURLM_CALL_MULTI_PERFORM);
+
+    /* Try to find done transfers, so we can free the easy
+     * handle again. */
+    do {
+        CURLMsg *msg;
+        msg = curl_multi_info_read(s->multi, &msgs_in_queue);
+
+        if (!msg)
+            break;
+        if (msg->msg == CURLMSG_NONE)
+            break;
+
+        switch (msg->msg) {
+            case CURLMSG_DONE:
+            {
+                HTTPState *state = NULL;
+                curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE, (char**)&state);
+
+                // Finish processing this chunk
+                qemu_iovec_from_buffer(state->acb->qiov, state->orig_buf, state->buf_len);
+                qemu_free(state->orig_buf);
+                state->acb->common.cb(state->acb->common.opaque, 0);
+                qemu_aio_release(state->acb);
+                state->acb = NULL;
+
+                http_clean_state(state);
+                break;
+            }
+            default:
+                msgs_in_queue = 0;
+                break;
+        }
+    } while(msgs_in_queue);
+#endif
+}
+
+static HTTPState *http_init_state(BDRVHTTPState *s)
+{
+    HTTPState *state = NULL;
+    int i;
+
+    do {
+        for (i=0; i<HTTP_NUM_STATES; i++) {
+            if (!s->states[i].in_use) {
+                state = &s->states[i];
+                state->in_use = 1;
+                break;
+            }
+        }
+        if (!state) {
+            usleep(100);
+            http_multi_do(s);
+        }
+    } while(!state);
+
+    if (state->curl)
+        goto has_curl;
+
+    state->curl = curl_easy_init();
+    if (!state->curl)
+        return NULL;
+    curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
+    curl_easy_setopt(state->curl, CURLOPT_TIMEOUT, 5);
+    curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, http_read_cb);
+    curl_easy_setopt(state->curl, CURLOPT_WRITEDATA, (void *)state);
+    curl_easy_setopt(state->curl, CURLOPT_PRIVATE, (void *)state);
+    curl_easy_setopt(state->curl, CURLOPT_AUTOREFERER, 1);
+    curl_easy_setopt(state->curl, CURLOPT_FOLLOWLOCATION, 1);
+    curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1);
+    curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg);
+    
+#ifdef DEBUG_VERBOSE
+    curl_easy_setopt(state->curl, CURLOPT_VERBOSE, 1);
+#endif
+
+has_curl:
+
+    state->s = s;
+
+    return state;
+}
+
+static void http_clean_state(HTTPState *s)
+{
+    if (s->s->multi)
+        curl_multi_remove_handle(s->s->multi, s->curl);
+    s->in_use = 0;
+}
+
+static int http_open(BlockDriverState *bs, const char *filename, int flags)
+{
+    BDRVHTTPState *s = bs->opaque;
+    HTTPState *state = NULL;
+    double d;
+    static int inited = 0;
+
+    if (!inited) {
+        curl_global_init(CURL_GLOBAL_ALL);
+        inited = 1;
+    }
+
+    printd("CURL: Opening %s\n", filename);
+    s->url = strdup(filename);
+    state = http_init_state(s);
+    if (!state)
+        goto out_noclean;
+
+    // Get file size
+
+    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
+    if (curl_easy_perform(state->curl))
+        goto out;
+    curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
+    curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
+    if (!d)
+        goto out;
+    printd("CURL: Size = %lld\n", (long long)d);
+    s->len = (int64_t)d;
+
+    http_clean_state(state);
+    curl_easy_cleanup(state->curl);
+    state->curl = NULL;
+
+#ifdef CONFIG_AIO
+    // Now we know the file exists and its size, so let's
+    // initialize the multi interface!
+
+    s->multi = curl_multi_init();
+    curl_multi_setopt( s->multi, CURLMOPT_SOCKETDATA, s); 
+    curl_multi_setopt( s->multi, CURLMOPT_SOCKETFUNCTION, http_sock_cb ); 
+    http_multi_do(s);
+#endif /* CONFIG_AIO */
+
+    return 0;
+
+out:
+    fprintf(stderr, "CURL: Error opening file: %s\n", state->errmsg);
+    curl_easy_cleanup(state->curl);
+    state->curl = NULL;
+out_noclean:
+    return -EINVAL;
+}
+
+#ifdef CONFIG_AIO
+
+static BlockDriverAIOCB *http_aio_readv(BlockDriverState *bs,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVHTTPState *s = bs->opaque;
+    HTTPAIOCB *acb;
+    long long start = sector_num * SECTOR_SIZE;
+    long long end = start + (nb_sectors * SECTOR_SIZE) - 1;
+    HTTPState *state;
+
+    acb = qemu_aio_get(bs, cb, opaque);
+    if (!acb)
+        return NULL;
+
+    state = http_init_state(s);
+    if (!state)
+        return NULL;
+
+    state->buf_len = (nb_sectors * SECTOR_SIZE);
+    state->buf = qemu_malloc(state->buf_len);
+    state->orig_buf = state->buf;
+    state->acb = acb;
+    acb->qiov = qiov;
+
+    snprintf(state->range, 127, "%lld-%lld", start, end);
+    printd("CURL (AIO): Reading %d at %lld (%s)\n", (nb_sectors * SECTOR_SIZE), start, state->range);
+    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
+
+    curl_multi_add_handle(s->multi, state->curl);
+    http_multi_do(s);
+
+    return &acb->common;
+}
+
+#else
+
+static int http_read(BlockDriverState *bs, int64_t sector_num,
+                    uint8_t *buf, int nb_sectors)
+{
+    BDRVHTTPState *s = bs->opaque;
+    CURLcode res;
+    long long start = sector_num * SECTOR_SIZE;
+    long long end = start + (nb_sectors * SECTOR_SIZE) - 1;
+    HTTPState *state;
+
+    state = http_init_state(s);
+    if (!state)
+        return -1;
+
+    state->buf = buf;
+
+    // seek(sector_num * SECTOR_SIZE);
+    snprintf(state->range, 127, "%lld-%lld", start, end);
+    printd("CURL: Reading %d at %lld (%s)\n", (nb_sectors * SECTOR_SIZE), start, state->range);
+    curl_easy_setopt(state->curl, CURLOPT_RANGE, state->range);
+
+    // read(nb_sectors * SECTOR_SIZE); -> buf
+    res = curl_easy_perform(state->curl);
+
+    http_clean_state(state);
+
+    printd("CURL: Done Reading\n");
+    return 0;
+}
+
+#endif /* CONFIG_AIO */
+
+static void http_close(BlockDriverState *bs)
+{
+    BDRVHTTPState *s = bs->opaque;
+    int i;
+
+    printd("CURL: Close\n");
+    for (i=0; i<HTTP_NUM_STATES; i++) {
+        if (s->states[i].in_use)
+            http_clean_state(&s->states[i]);
+        if (s->states[i].curl) {
+            curl_easy_cleanup(s->states[i].curl);
+            s->states[i].curl = NULL;
+        }
+    }
+    if (s->multi)
+        curl_multi_cleanup(s->multi);
+}
+
+static int64_t http_getlength(BlockDriverState *bs)
+{
+    BDRVHTTPState *s = bs->opaque;
+    return s->len;
+}
+
+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,
+
+#ifdef CONFIG_AIO
+    .aiocb_size		= sizeof(HTTPAIOCB),
+    .bdrv_aio_readv	= http_aio_readv,
+#else
+    .bdrv_read		= http_read,
+#endif
+};
diff --git a/block.c b/block.c
index 3d1223d..76a8d86 100644
--- a/block.c
+++ b/block.c
@@ -1504,6 +1504,9 @@ void bdrv_init(void)
     bdrv_register(&bdrv_qcow2);
     bdrv_register(&bdrv_parallels);
     bdrv_register(&bdrv_nbd);
+#ifdef CONFIG_CURL
+    bdrv_register(&bdrv_http);
+#endif
 }
 
 void aio_pool_init(AIOPool *pool, int aiocb_size,
diff --git a/block.h b/block.h
index 5aef076..ebff06d 100644
--- a/block.h
+++ b/block.h
@@ -20,6 +20,7 @@ extern BlockDriver bdrv_vvfat;
 extern BlockDriver bdrv_qcow2;
 extern BlockDriver bdrv_parallels;
 extern BlockDriver bdrv_nbd;
+extern BlockDriver bdrv_http;
 
 typedef struct BlockDriverInfo {
     /* in bytes, 0 if irrelevant */
diff --git a/configure b/configure
index 82fb60a..49b5b84 100755
--- a/configure
+++ b/configure
@@ -181,6 +181,7 @@ bsd_user="no"
 build_docs="no"
 uname_release=""
 curses="yes"
+curl="yes"
 pthread="yes"
 aio="yes"
 io_thread="no"
@@ -477,6 +478,8 @@ for opt do
   ;;
   --disable-curses) curses="no"
   ;;
+  --disable-curl) curl="no"
+  ;;
   --disable-nptl) nptl="no"
   ;;
   --enable-mixemu) mixemu="yes"
@@ -600,6 +603,7 @@ echo "  --disable-brlapi         disable BrlAPI"
 echo "  --disable-vnc-tls        disable TLS encryption for VNC server"
 echo "  --disable-vnc-sasl       disable SASL encryption for VNC server"
 echo "  --disable-curses         disable curses output"
+echo "  --disable-curl           disable curl connectivity"
 echo "  --disable-bluez          disable bluez stack connectivity"
 echo "  --disable-kvm            disable KVM acceleration support"
 echo "  --disable-nptl           disable usermode NPTL support"
@@ -1062,6 +1066,21 @@ EOF
 fi # test "$curses"
 
 ##########################################
+# curl probe
+
+if test "$curl" = "yes" ; then
+  curl=no
+  cat > $TMPC << EOF
+#include <curl/curl.h>
+int main(void) { return curl_easy_init(); }
+EOF
+  curl_libs=`curl-config --libs`
+ if $cc $ARCH_CFLAGS $curl_libs -o $TMPE $TMPC > /dev/null 2> /dev/null ; then
+    curl=yes
+  fi
+fi # test "$curl"
+
+##########################################
 # bluez support probe
 if test "$bluez" = "yes" ; then
   `pkg-config bluez 2> /dev/null` || bluez="no"
@@ -1311,6 +1330,7 @@ if test "$sdl" != "no" ; then
     echo "SDL static link   $sdl_static"
 fi
 echo "curses support    $curses"
+echo "curl support      $curl"
 echo "mingw32 support   $mingw32"
 echo "Audio drivers     $audio_drv_list"
 echo "Extra audio cards $audio_card_list"
@@ -1637,6 +1657,11 @@ fi
 if test "$inotify" = "yes" ; then
   echo "#define CONFIG_INOTIFY 1" >> $config_h
 fi
+if test "$curl" = "yes" ; then
+  echo "CONFIG_CURL=yes" >> $config_mak
+  echo "CURL_LIBS=$curl_libs" >> $config_mak
+  echo "#define CONFIG_CURL 1" >> $config_h
+fi
 if test "$brlapi" = "yes" ; then
   echo "CONFIG_BRLAPI=yes" >> $config_mak
   echo "#define CONFIG_BRLAPI 1" >> $config_h
-- 
1.6.0.2

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

[Daniel P. Berrange]

On Wed, May 06, 2009 at 03:00:50AM +0200, alex@csgraf.de wrote:
> From: Alexander Graf <alex@csgraf.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

I rather think there should be an explicit flag to allow use of http://
URLs in filenames at runtime, not just 'configure' time. There are many
apps out there using QEMU which will be assuming QEMU treats all disk
paths as local files, and thus not got explicit code to check whether 
a URI is passed. I could well see that some will consider it a security
issue to allow QEMU to download off the net, but if they updated to
a new QEMU with this patch, downloading would be allowed by default.

Perhaps only enable these remote URIs with the -drive parameter, when
an explicit fmt=http option is set. But can this be layered into the
other protocols, eg could the remote URI be in qcow, vmdk, etc formats,
or are you assuming the remote uri is raw file ?

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Kevin Wolf]

Daniel P. Berrange schrieb:
> On Wed, May 06, 2009 at 03:00:50AM +0200, alex@csgraf.de wrote:
>> From: Alexander Graf <alex@csgraf.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
> 
> I rather think there should be an explicit flag to allow use of http://
> URLs in filenames at runtime, not just 'configure' time. There are many
> apps out there using QEMU which will be assuming QEMU treats all disk
> paths as local files, and thus not got explicit code to check whether 
> a URI is passed. I could well see that some will consider it a security
> issue to allow QEMU to download off the net, but if they updated to
> a new QEMU with this patch, downloading would be allowed by default.

If apps want to be sure that they are accessing a local file, they must
ensure not to have a colon in the file name. Otherwise this specifies a
protocol for the qemu block layer.

Btw, we could use a way to escape colons in a file name. Using such
files isn't possible currently.

> Perhaps only enable these remote URIs with the -drive parameter, when
> an explicit fmt=http option is set. But can this be layered into the
> other protocols, eg could the remote URI be in qcow, vmdk, etc formats,
> or are you assuming the remote uri is raw file ?

It should work with all formats. This is why fmt=http is wrong. It's not
a format, but a protocol.

Kevin

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

[Daniel P. Berrange]

On Wed, May 06, 2009 at 11:14:17AM +0200, Kevin Wolf wrote:
> Daniel P. Berrange schrieb:
> > On Wed, May 06, 2009 at 03:00:50AM +0200, alex@csgraf.de wrote:
> >> From: Alexander Graf <alex@csgraf.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
> > 
> > I rather think there should be an explicit flag to allow use of http://
> > URLs in filenames at runtime, not just 'configure' time. There are many
> > apps out there using QEMU which will be assuming QEMU treats all disk
> > paths as local files, and thus not got explicit code to check whether 
> > a URI is passed. I could well see that some will consider it a security
> > issue to allow QEMU to download off the net, but if they updated to
> > a new QEMU with this patch, downloading would be allowed by default.
> 
> If apps want to be sure that they are accessing a local file, they must
> ensure not to have a colon in the file name. Otherwise this specifies a
> protocol for the qemu block layer.

Using paths with a colon in the file name is pretty critical if
you wish to configure VMs with block devices using a stable path
like those under /dev/disk/by-path/ - they pretty much all contain
colons, and are the only good stable path for iSCSI or FibreChannel
block devices.

> Btw, we could use a way to escape colons in a file name. Using such
> files isn't possible currently.
> 
> > Perhaps only enable these remote URIs with the -drive parameter, when
> > an explicit fmt=http option is set. But can this be layered into the
> > other protocols, eg could the remote URI be in qcow, vmdk, etc formats,
> > or are you assuming the remote uri is raw file ?
> 
> It should work with all formats. This is why fmt=http is wrong. It's not
> a format, but a protocol.

Then I'd prefer we add a protocol=XXX option for magic protocols. This 
would be easier to use & clearer than requiring escaping of magic 
characters, eg

   -drive file=/some/path:with:colons,protocol=file
   -drive file=http://some/path,protocol=uri

For compatability, we could make it such that if protocol=XXX was left out,
it could try and "guess" it, in same way QEMU does if format=XXX is left out
for content format.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Alexander Graf]

On 06.05.2009, at 11:31, Daniel P. Berrange wrote:
> On Wed, May 06, 2009 at 11:14:17AM +0200, Kevin Wolf wrote:
>> Daniel P. Berrange schrieb:
>>> Perhaps only enable these remote URIs with the -drive parameter,  
>>> when
>>> an explicit fmt=http option is set. But can this be layered into the
>>> other protocols, eg could the remote URI be in qcow, vmdk, etc  
>>> formats,
>>> or are you assuming the remote uri is raw file ?
>>
>> It should work with all formats. This is why fmt=http is wrong.  
>> It's not
>> a format, but a protocol.
>
> Then I'd prefer we add a protocol=XXX option for magic protocols. This
> would be easier to use & clearer than requiring escaping of magic
> characters, eg
>
>   -drive file=/some/path:with:colons,protocol=file
>   -drive file=http://some/path,protocol=uri
>
> For compatability, we could make it such that if protocol=XXX was  
> left out,
> it could try and "guess" it, in same way QEMU does if format=XXX is  
> left out
> for content format.

I like that idea, yes.

Unfortunately it is out of scope of this patch. The problem you're  
describing with qemu taking on remote connections exists today already  
with NBD support. Right now there are 2 protocols implemented in qemu  
that I'm aware of:

- file (local)
- nbd (remote)

and I just add another remote one:
- http (remote)

So your concerns do apply today already. Maybe you want to come up  
with a patch to implement a protocol= parameter to -drive?

Alex

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

[Avi Kivity]

Daniel P. Berrange wrote:
> Then I'd prefer we add a protocol=XXX option for magic protocols. This 
> would be easier to use & clearer than requiring escaping of magic 
> characters, eg
>
>    -drive file=/some/path:with:colons,protocol=file
>    -drive file=http://some/path,protocol=uri
>
> For compatability, we could make it such that if protocol=XXX was left out,
> it could try and "guess" it, in same way QEMU does if format=XXX is left out
> for content format.
>   

How about

  -drive file=file:///some/path:with:colons?

Libvirt would use this unconditionally, command-line users can choose.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Kevin Wolf]

Avi Kivity schrieb:
> Daniel P. Berrange wrote:
>> Then I'd prefer we add a protocol=XXX option for magic protocols. This 
>> would be easier to use & clearer than requiring escaping of magic 
>> characters, eg
>>
>>    -drive file=/some/path:with:colons,protocol=file
>>    -drive file=http://some/path,protocol=uri
>>
>> For compatability, we could make it such that if protocol=XXX was left out,
>> it could try and "guess" it, in same way QEMU does if format=XXX is left out
>> for content format.
>>   
> 
> How about
> 
>   -drive file=file:///some/path:with:colons?
> 
> Libvirt would use this unconditionally, command-line users can choose.

+1

The patch to achieve this would be pretty easy. You just need to assign
the file protocol to bdrv_raw and have it strip the file: prefix in its
open function.

Kevin

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

[Daniel P. Berrange]

On Wed, May 06, 2009 at 12:57:26PM +0300, Avi Kivity wrote:
> Daniel P. Berrange wrote:
> >Then I'd prefer we add a protocol=XXX option for magic protocols. This 
> >would be easier to use & clearer than requiring escaping of magic 
> >characters, eg
> >
> >   -drive file=/some/path:with:colons,protocol=file
> >   -drive file=http://some/path,protocol=uri
> >
> >For compatability, we could make it such that if protocol=XXX was left out,
> >it could try and "guess" it, in same way QEMU does if format=XXX is left 
> >out
> >for content format.
> >  
> 
> How about
> 
>  -drive file=file:///some/path:with:colons?
> 
> Libvirt would use this unconditionally, command-line users can choose.

Sure, that achieves the same end result, so fine by me.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Daniel P. Berrange]

On Wed, May 06, 2009 at 11:06:28AM +0100, Daniel P. Berrange wrote:
> On Wed, May 06, 2009 at 12:57:26PM +0300, Avi Kivity wrote:
> > Daniel P. Berrange wrote:
> > >Then I'd prefer we add a protocol=XXX option for magic protocols. This 
> > >would be easier to use & clearer than requiring escaping of magic 
> > >characters, eg
> > >
> > >   -drive file=/some/path:with:colons,protocol=file
> > >   -drive file=http://some/path,protocol=uri
> > >
> > >For compatability, we could make it such that if protocol=XXX was left out,
> > >it could try and "guess" it, in same way QEMU does if format=XXX is left 
> > >out
> > >for content format.
> > >  
> > 
> > How about
> > 
> >  -drive file=file:///some/path:with:colons?
> > 
> > Libvirt would use this unconditionally, command-line users can choose.
> 
> Sure, that achieves the same end result, so fine by me.

Oh and if you want to be really nice, you could say that any filename
with a leading '/' is implicitly file:///, since I can't imagine
any protocol name starting with a  '/'.  And this would ensure any
existing usage with absolute filenames 'just works' without tripping
up on colons.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Avi Kivity]

Daniel P. Berrange wrote:
> Oh and if you want to be really nice, you could say that any filename
> with a leading '/' is implicitly file:///, since I can't imagine
> any protocol name starting with a  '/'.  And this would ensure any
> existing usage with absolute filenames 'just works' without tripping
> up on colons.
>   

A protocol should match ^[a-zA-Z][a-zA-Z0-9_-]*://, anything else is a file.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Alexander Graf]

On 06.05.2009, at 12:13, Daniel P. Berrange wrote:

> On Wed, May 06, 2009 at 11:06:28AM +0100, Daniel P. Berrange wrote:
>> On Wed, May 06, 2009 at 12:57:26PM +0300, Avi Kivity wrote:
>>> Daniel P. Berrange wrote:
>>>> Then I'd prefer we add a protocol=XXX option for magic protocols.  
>>>> This
>>>> would be easier to use & clearer than requiring escaping of magic
>>>> characters, eg
>>>>
>>>>  -drive file=/some/path:with:colons,protocol=file
>>>>  -drive file=http://some/path,protocol=uri
>>>>
>>>> For compatability, we could make it such that if protocol=XXX was  
>>>> left out,
>>>> it could try and "guess" it, in same way QEMU does if format=XXX  
>>>> is left
>>>> out
>>>> for content format.
>>>>
>>>
>>> How about
>>>
>>> -drive file=file:///some/path:with:colons?
>>>
>>> Libvirt would use this unconditionally, command-line users can  
>>> choose.
>>
>> Sure, that achieves the same end result, so fine by me.
>
> Oh and if you want to be really nice, you could say that any filename
> with a leading '/' is implicitly file:///, since I can't imagine
> any protocol name starting with a  '/'.  And this would ensure any
> existing usage with absolute filenames 'just works' without tripping
> up on colons.

No, "the right way" would be to always assume file:// for filenames  
that don't have a protocol ("://") specified.

Alex

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

[Anthony Liguori]

Alexander Graf wrote:
>
> On 06.05.2009, at 12:13, Daniel P. Berrange wrote:
>
>> On Wed, May 06, 2009 at 11:06:28AM +0100, Daniel P. Berrange wrote:
>>> On Wed, May 06, 2009 at 12:57:26PM +0300, Avi Kivity wrote:
>>>> Daniel P. Berrange wrote:
>>>>> Then I'd prefer we add a protocol=XXX option for magic protocols. 
>>>>> This
>>>>> would be easier to use & clearer than requiring escaping of magic
>>>>> characters, eg
>>>>>
>>>>>  -drive file=/some/path:with:colons,protocol=file
>>>>>  -drive file=http://some/path,protocol=uri
>>>>>
>>>>> For compatability, we could make it such that if protocol=XXX was 
>>>>> left out,
>>>>> it could try and "guess" it, in same way QEMU does if format=XXX 
>>>>> is left
>>>>> out
>>>>> for content format.
>>>>>
>>>>
>>>> How about
>>>>
>>>> -drive file=file:///some/path:with:colons?
>>>>
>>>> Libvirt would use this unconditionally, command-line users can choose.
>>>
>>> Sure, that achieves the same end result, so fine by me.
>>
>> Oh and if you want to be really nice, you could say that any filename
>> with a leading '/' is implicitly file:///, since I can't imagine
>> any protocol name starting with a  '/'.  And this would ensure any
>> existing usage with absolute filenames 'just works' without tripping
>> up on colons.
>
> No, "the right way" would be to always assume file:// for filenames 
> that don't have a protocol ("://") specified.

nbd doesn't use '://'.  It's prohibitively hard to express a unix domain 
socket and parameters with a standard URI.

Regards,

Anthony Liguori

> Alex
>
>
>

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

[Kevin Wolf]

Daniel P. Berrange schrieb:
> Oh and if you want to be really nice, you could say that any filename
> with a leading '/' is implicitly file:///, since I can't imagine
> any protocol name starting with a  '/'.  And this would ensure any
> existing usage with absolute filenames 'just works' without tripping
> up on colons.

Existing usage would mean to get an error message. I don't see any
reason why we should change this behaviour if we have a real solution.
Adding more exceptions to the guessing doesn't really make it more harmless.

Kevin

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

[Alexander Graf]

On 06.05.2009, at 12:16, Kevin Wolf wrote:

> Daniel P. Berrange schrieb:
>> Oh and if you want to be really nice, you could say that any filename
>> with a leading '/' is implicitly file:///, since I can't imagine
>> any protocol name starting with a  '/'.  And this would ensure any
>> existing usage with absolute filenames 'just works' without tripping
>> up on colons.
>
> Existing usage would mean to get an error message. I don't see any
> reason why we should change this behaviour if we have a real solution.
> Adding more exceptions to the guessing doesn't really make it more  
> harmless.

No, please don't break something as essential as this without a good  
reason. If you don't specify a protocol, chances are 99.999% the user  
wants a file, so let's behave that way!

Alex

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

[Daniel P. Berrange]

On Wed, May 06, 2009 at 12:16:01PM +0200, Kevin Wolf wrote:
> Daniel P. Berrange schrieb:
> > Oh and if you want to be really nice, you could say that any filename
> > with a leading '/' is implicitly file:///, since I can't imagine
> > any protocol name starting with a  '/'.  And this would ensure any
> > existing usage with absolute filenames 'just works' without tripping
> > up on colons.
> 
> Existing usage would mean to get an error message. I don't see any
> reason why we should change this behaviour if we have a real solution.
> Adding more exceptions to the guessing doesn't really make it more harmless.

By existing usage, I meant the practice of supplying an absolute filename
to QEMU, which works all the time, except if you're unlucky to have a colon
in the filename, in which case it breaks, even though it could  trivially
work. Getting an error for an absolute path with a colon is a bug IMHO,
rather than useful behaviour.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Alexander Graf]

On 06.05.2009, at 11:57, Avi Kivity wrote:

> Daniel P. Berrange wrote:
>> Then I'd prefer we add a protocol=XXX option for magic protocols.  
>> This would be easier to use & clearer than requiring escaping of  
>> magic characters, eg
>>
>>   -drive file=/some/path:with:colons,protocol=file
>>   -drive file=http://some/path,protocol=uri
>>
>> For compatability, we could make it such that if protocol=XXX was  
>> left out,
>> it could try and "guess" it, in same way QEMU does if format=XXX is  
>> left out
>> for content format.
>>
>
> How about
>
> -drive file=file:///some/path:with:colons?
>
> Libvirt would use this unconditionally, command-line users can choose.

It's about the same as having a protocol= parameter, but more  
compatible and easier. I'm sold :-).

Alex

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

[Anthony Liguori]

Avi Kivity wrote:
> Daniel P. Berrange wrote:
>> Then I'd prefer we add a protocol=XXX option for magic protocols. 
>> This would be easier to use & clearer than requiring escaping of 
>> magic characters, eg
>>
>>    -drive file=/some/path:with:colons,protocol=file
>>    -drive file=http://some/path,protocol=uri
>>
>> For compatability, we could make it such that if protocol=XXX was 
>> left out,
>> it could try and "guess" it, in same way QEMU does if format=XXX is 
>> left out
>> for content format.
>>   
>
> How about
>
>  -drive file=file:///some/path:with:colons?

No '///' please.

Regards,

Anthony Liguori

> Libvirt would use this unconditionally, command-line users can choose.
>

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

[Avi Kivity]

Anthony Liguori wrote:
>>
>>  -drive file=file:///some/path:with:colons?
>
> No '///' please.
>

Why not?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>>
>>>  -drive file=file:///some/path:with:colons?
>>
>> No '///' please.
>>
>
> Why not?

For better or worse, this aren't URIs.  The main reason, is that a URI 
requires a hostname as the first component (which is empty in your 
URI).  But unix domain sockets don't match up with URIs because they 
aren't hostnames.

Regards,

Anthony Liguori

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

[François Revol]

> Avi Kivity wrote:
> > Anthony Liguori wrote:
> >>>
> >>>  -drive file=file:///some/path:with:colons?
> >>
> >> No '///' please.
> >>
> >
> > Why not?
>
> For better or worse, this aren't URIs.  The main reason, is that a
> URI
> requires a hostname as the first component (which is empty in your
> URI).  But unix domain sockets don't match up with URIs because they
> aren't hostnames.

Not for file:, which is why there are 3 /, though it's actually *
possible* to specify a hostname for file:, it's *not* required, and
useless anyway.

François.

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

[Anthony Liguori]

Avi Kivity wrote:
> Daniel P. Berrange wrote:
>> Then I'd prefer we add a protocol=XXX option for magic protocols. 
>> This would be easier to use & clearer than requiring escaping of 
>> magic characters, eg
>>
>>    -drive file=/some/path:with:colons,protocol=file
>>    -drive file=http://some/path,protocol=uri
>>
>> For compatability, we could make it such that if protocol=XXX was 
>> left out,
>> it could try and "guess" it, in same way QEMU does if format=XXX is 
>> left out
>> for content format.
>>   
>
> How about
>
>  -drive file=file:///some/path:with:colons?
>
> Libvirt would use this unconditionally, command-line users can choose.

Why does libvirt care?  That confuses me.

Regards,

Anthony Liguori

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

[Daniel P. Berrange]

On Wed, May 06, 2009 at 08:12:56AM -0500, Anthony Liguori wrote:
> Avi Kivity wrote:
> >Daniel P. Berrange wrote:
> >>Then I'd prefer we add a protocol=XXX option for magic protocols. 
> >>This would be easier to use & clearer than requiring escaping of 
> >>magic characters, eg
> >>
> >>   -drive file=/some/path:with:colons,protocol=file
> >>   -drive file=http://some/path,protocol=uri
> >>
> >>For compatability, we could make it such that if protocol=XXX was 
> >>left out,
> >>it could try and "guess" it, in same way QEMU does if format=XXX is 
> >>left out
> >>for content format.
> >>  
> >
> >How about
> >
> > -drive file=file:///some/path:with:colons?
> >
> >Libvirt would use this unconditionally, command-line users can choose.
> 
> Why does libvirt care?  That confuses me.

I don't have any problem with QEMU supporting http, nbd, or other clever
file access schemes. I'll let others debate its merits vs using FUSE 
http filesystems :-)

All I care about from libvirt POV, is that there is a way to give QEMU an 
absolute file path for a disk, and guarentee that QEMU will treat this as
a local file path, and not try any access protocols, other than 'open(2)'.
Various options from this thread....

 - Treat any path starting with / as local file
 - Allow file:  as a prefix
 - Allow file:/// as a prefix, real URI style
 - Add a protocol=file  flag to -drive

Pick one, pick several, suggest more. Any of these options would work as 
far as I'm concerned, and we could easily support several. I think the
first is desirable because that's the natural thing users will try when
launching QEMU directly. I see the merit in also having file: or file:///
as an explicit protocol too. I think the 4th protocol=file is redundant
really, given that we have precedent of using XXX: prefixes in nbd driver

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, May 06, 2009 at 08:12:56AM -0500, Anthony Liguori wrote:
>   
>> Avi Kivity wrote:
>>     
>>> Daniel P. Berrange wrote:
>>>       
>>>> Then I'd prefer we add a protocol=XXX option for magic protocols. 
>>>> This would be easier to use & clearer than requiring escaping of 
>>>> magic characters, eg
>>>>
>>>>   -drive file=/some/path:with:colons,protocol=file
>>>>   -drive file=http://some/path,protocol=uri
>>>>
>>>> For compatability, we could make it such that if protocol=XXX was 
>>>> left out,
>>>> it could try and "guess" it, in same way QEMU does if format=XXX is 
>>>> left out
>>>> for content format.
>>>>  
>>>>         
>>> How about
>>>
>>> -drive file=file:///some/path:with:colons?
>>>
>>> Libvirt would use this unconditionally, command-line users can choose.
>>>       
>> Why does libvirt care?  That confuses me.
>>     
>
> I don't have any problem with QEMU supporting http, nbd, or other clever
> file access schemes. I'll let others debate its merits vs using FUSE 
> http filesystems :-)
>
> All I care about from libvirt POV, is that there is a way to give QEMU an 
> absolute file path for a disk, and guarentee that QEMU will treat this as
> a local file path, and not try any access protocols, other than 'open(2)'.
> Various options from this thread....
>
>  - Treat any path starting with / as local file
>  - Allow file:  as a prefix
>  - Allow file:/// as a prefix, real URI style
>  - Add a protocol=file  flag to -drive
>   
The only viable solution is for libvirt to escape it's file names.

For instance, you're in all sorts of trouble if a user creates a 
filename that looks like 'My disk image,format=raw.img'

Regards,

Anthony Liguori

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

[Kevin Wolf]

Daniel P. Berrange schrieb:
> All I care about from libvirt POV, is that there is a way to give QEMU an 
> absolute file path for a disk, and guarentee that QEMU will treat this as
> a local file path, and not try any access protocols, other than 'open(2)'.
> Various options from this thread....
> 
>  - Treat any path starting with / as local file
>  - Allow file:  as a prefix
>  - Allow file:/// as a prefix, real URI style
>  - Add a protocol=file  flag to -drive
> 
> Pick one, pick several, suggest more. Any of these options would work as 
> far as I'm concerned, and we could easily support several. I think the
> first is desirable because that's the natural thing users will try when
> launching QEMU directly.

I think we all agree that file: is a reasonable option for libvirt which
covers all cases of colons (even relative paths starting with http:), so
let's take this one. If you want to magically do the right thing for
users invoking qemu manually, I think we should rather go for:

5) Treat anything as local file which has a protocol prefix that doesn't
match a known protocol

This would mean that vvfat:xyz uses the vvfat protocol, but foo:bar is a
local file because the protocol foo doesn't exist. I think this variant
is less confusing magic than checking for a completely unrelated
character like /.

Kevin

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

[Anthony Liguori]

Kevin Wolf wrote:
> Daniel P. Berrange schrieb:
>   
>> All I care about from libvirt POV, is that there is a way to give QEMU an 
>> absolute file path for a disk, and guarentee that QEMU will treat this as
>> a local file path, and not try any access protocols, other than 'open(2)'.
>> Various options from this thread....
>>
>>  - Treat any path starting with / as local file
>>  - Allow file:  as a prefix
>>  - Allow file:/// as a prefix, real URI style
>>  - Add a protocol=file  flag to -drive
>>
>> Pick one, pick several, suggest more. Any of these options would work as 
>> far as I'm concerned, and we could easily support several. I think the
>> first is desirable because that's the natural thing users will try when
>> launching QEMU directly.
>>     
>
> I think we all agree that file: is a reasonable option for libvirt which
> covers all cases of colons (even relative paths starting with http:), so
> let's take this one. If you want to magically do the right thing for
> users invoking qemu manually, I think we should rather go for:
>   

I've changed my mind.  I think file: makes the situation worse because 
it gives a false sense of security.  If you just pass 
file:<user-inputed-filename> you are broken.  You still have to escape 
commas.  If you're escaping commas, you might as well escape colons too.

I think escaping is the only complete solution.

Regards,

Anthony Liguori

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

[Kevin Wolf]

Anthony Liguori schrieb:
> I've changed my mind.  I think file: makes the situation worse because 
> it gives a false sense of security.  If you just pass 
> file:<user-inputed-filename> you are broken.  You still have to escape 
> commas.  If you're escaping commas, you might as well escape colons too.
> 
> I think escaping is the only complete solution.

You're completely right if we don't limit the discussion to colons. I'm
not sure if breaking colons really makes tools aware that they need to
take care of commas, but "it's broken anyway" is a reasonable answer if
we can have a generic fix (and we can have it here).

So what should the generic escaping look like?

And I still think we should do something about colons, they are much
more common in paths than commas. Even more so on Windows, I guess.
Should we still implement the strategy "anything that isn't a valid
protocol must be a local file"?

Kevin

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

[Anthony Liguori]

Kevin Wolf wrote:
> Anthony Liguori schrieb:
>   
>> I've changed my mind.  I think file: makes the situation worse because 
>> it gives a false sense of security.  If you just pass 
>> file:<user-inputed-filename> you are broken.  You still have to escape 
>> commas.  If you're escaping commas, you might as well escape colons too.
>>
>> I think escaping is the only complete solution.
>>     
>
> You're completely right if we don't limit the discussion to colons. I'm
> not sure if breaking colons really makes tools aware that they need to
> take care of commas, but "it's broken anyway" is a reasonable answer if
> we can have a generic fix (and we can have it here).
>   

If we have escaping, I'm happy to consider file: if people still think 
it's useful.  But adding file: without escaping is IMHO dangerous.

> So what should the generic escaping look like?
>   

-drive file=my-silly\:filename\,with\ strange\ characters

I see no reason to do anything overly complicated.  Of course, if you're 
in a shell, you'll have to double escape unless you've got single quotes 
around it.  That's a good argument for file: in addition to escaping.

Regards,

Anthony Liguori

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

[Alexander Graf]

On 06.05.2009, at 16:39, Anthony Liguori wrote:

> Kevin Wolf wrote:
>> Anthony Liguori schrieb:
>>
>>> I've changed my mind.  I think file: makes the situation worse  
>>> because it gives a false sense of security.  If you just pass  
>>> file:<user-inputed-filename> you are broken.  You still have to  
>>> escape commas.  If you're escaping commas, you might as well  
>>> escape colons too.
>>>
>>> I think escaping is the only complete solution.
>>>
>>
>> You're completely right if we don't limit the discussion to colons.  
>> I'm
>> not sure if breaking colons really makes tools aware that they need  
>> to
>> take care of commas, but "it's broken anyway" is a reasonable  
>> answer if
>> we can have a generic fix (and we can have it here).
>>
>
> If we have escaping, I'm happy to consider file: if people still  
> think it's useful.  But adding file: without escaping is IMHO  
> dangerous.
>
>> So what should the generic escaping look like?
>>
>
> -drive file=my-silly\:filename\,with\ strange\ characters

I liked the idea of URI escaping, like "%20" being byte 32 -> space.  
There are enough libraries out there that can easily generate such an  
escaping, so people don't have to stick with a dozen of different ways  
of escaping, depending on which option they're using.

Alex

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

[Avi Kivity]

Anthony Liguori wrote:
>> So what should the generic escaping look like?
>>   
>
> -drive file=my-silly\:filename\,with\ strange\ characters
>
> I see no reason to do anything overly complicated.  Of course, if 
> you're in a shell, you'll have to double escape unless you've got 
> single quotes around it.  That's a good argument for file: in addition 
> to escaping.
>

With backslash escaping, I never know how many backslashes I end up 
with.  They're also the path separators in one OS.

I'd go with URL separating:

 -drive file=file:my-silly%3afilename%2cwith%20strange%20characters

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>> So what should the generic escaping look like?
>>>   
>>
>> -drive file=my-silly\:filename\,with\ strange\ characters
>>
>> I see no reason to do anything overly complicated.  Of course, if 
>> you're in a shell, you'll have to double escape unless you've got 
>> single quotes around it.  That's a good argument for file: in 
>> addition to escaping.
>>
>
> With backslash escaping, I never know how many backslashes I end up 
> with.  They're also the path separators in one OS.
>
> I'd go with URL separating:
>
> -drive file=file:my-silly%3afilename%2cwith%20strange%20characters

URI escaping has some pretty strange rules because there set of 
characters allowed in a URI is << what is representable in a shell 
without requiring escaping.

But honestly, I don't care that much.  I'll leave the decision up to 
whoever writes up the patch.

Regards,

Anthony Liguori

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

[Jamie Lokier]

Anthony Liguori wrote:
> Avi Kivity wrote:
> >Anthony Liguori wrote:
> >>>So what should the generic escaping look like?
> >>>  
> >>
> >>-drive file=my-silly\:filename\,with\ strange\ characters
> >>
> >>I see no reason to do anything overly complicated.  Of course, if 
> >>you're in a shell, you'll have to double escape unless you've got 
> >>single quotes around it.  That's a good argument for file: in 
> >>addition to escaping.
> >>
> >
> >With backslash escaping, I never know how many backslashes I end up 
> >with.  They're also the path separators in one OS.
> >
> >I'd go with URL separating:
> >
> >-drive file=file:my-silly%3afilename%2cwith%20strange%20characters
> 
> URI escaping has some pretty strange rules because there set of 
> characters allowed in a URI is << what is representable in a shell 
> without requiring escaping.

There's no harm in accepting characters which aren't normally allowed
in a URI.  When _escaped_, any octet sequence can be encoded in a URI
so there's no problem there.

Backslash quoting in shell is easiest to type like this, using single
quotes around it:

    -drive 'file=my-silly\:filename\,with\ strange\ characters'

Backslash quoting is easier to do in shell scripts which are driving
QEMU, or driving QEMU's monitor.  I.e. using sed or printf %q in Bash:

    qpath=`printf %q "$path"`
    qemu -drive file="$qpath"

(I don't know why, but conveniently Bash's %q quotes commas).

-- Jamie

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

[Kevin Wolf]

Anthony Liguori schrieb:
> -drive file=my-silly\:filename\,with\ strange\ characters
> 
> I see no reason to do anything overly complicated.  Of course, if you're 
> in a shell, you'll have to double escape unless you've got single quotes 
> around it.  That's a good argument for file: in addition to escaping.

Double escaping is why I thought that it might be ugly. But as long as
it's meant for tools and users still have something like file: I'm fine
with it.

Fortunately cmd.exe doesn't do escaping. Not having to use it, I must
admit that I would kind of like things like -hda c\\:\\\\foo.img though. ;-)

Kevin

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, May 06, 2009 at 11:14:17AM +0200, Kevin Wolf wrote:
>   
>> Daniel P. Berrange schrieb:
>>     
>>> On Wed, May 06, 2009 at 03:00:50AM +0200, alex@csgraf.de wrote:
>>>       
>>>> From: Alexander Graf <alex@csgraf.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
>>>>         
>>> I rather think there should be an explicit flag to allow use of http://
>>> URLs in filenames at runtime, not just 'configure' time. There are many
>>> apps out there using QEMU which will be assuming QEMU treats all disk
>>> paths as local files, and thus not got explicit code to check whether 
>>> a URI is passed. I could well see that some will consider it a security
>>> issue to allow QEMU to download off the net, but if they updated to
>>> a new QEMU with this patch, downloading would be allowed by default.
>>>       
>> If apps want to be sure that they are accessing a local file, they must
>> ensure not to have a colon in the file name. Otherwise this specifies a
>> protocol for the qemu block layer.
>>     
>
> Using paths with a colon in the file name is pretty critical if
> you wish to configure VMs with block devices using a stable path
> like those under /dev/disk/by-path/ - they pretty much all contain
> colons, and are the only good stable path for iSCSI or FibreChannel
> block devices.
>
>   
>> Btw, we could use a way to escape colons in a file name. Using such
>> files isn't possible currently.
>>
>>     
>>> Perhaps only enable these remote URIs with the -drive parameter, when
>>> an explicit fmt=http option is set. But can this be layered into the
>>> other protocols, eg could the remote URI be in qcow, vmdk, etc formats,
>>> or are you assuming the remote uri is raw file ?
>>>       
>> It should work with all formats. This is why fmt=http is wrong. It's not
>> a format, but a protocol.
>>     
>
> Then I'd prefer we add a protocol=XXX option for magic protocols. This 
> would be easier to use & clearer than requiring escaping of magic 
> characters, eg
>   

It just exacerbates the problem.  The comma is also a special character 
so now to avoid problems with ':' as a special character, you're relying 
on another special character.

We absolutely need escaping.  I have no doubt about that.

 From a human readable perspective though, I think we could introduce a 
"file:" protocol which ensured that we used a local file protocol.  For 
instance,

 -drive file=file:/some/path

Regards,

Anthony Liguori

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

[Jamie Lokier]

Kevin Wolf wrote:
> If apps want to be sure that they are accessing a local file, they must
> ensure not to have a colon in the file name. Otherwise this specifies a
> protocol for the qemu block layer.
> 
> Btw, we could use a way to escape colons in a file name. Using such
> files isn't possible currently.

Escaping:

Because I'm not sure how QEMU parses colons, spaces, commas, quotes
and who knows what other special characters in these options, and
because the monitor might use different parsing when reading a disk
image name, my management scripts always create symlinks to the disks,
and pass the names of the symlinks instead to the command line and the
monitor.

It's quite awkward for qcow2 delta files, because they contain the
name of their backing file, it can be relative, cannot be overriden,
and is resolved relative to the management script's symlinks, and they
can be stacked more than one deep...  But most of the time it is
solvable by making a set of symlinks in a little directory hierarchy.

I'd be quite happy with file:///path/to/file%20containing%20spaces.vhd
working, on the command line and in the monitor, in the the way file
URIs are meant to work.

-- Jamie

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, May 06, 2009 at 03:00:50AM +0200, alex@csgraf.de wrote:
>   
>> From: Alexander Graf <alex@csgraf.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
>>     
>
> I rather think there should be an explicit flag to allow use of http://
> URLs in filenames at runtime, not just 'configure' time. There are many
> apps out there using QEMU which will be assuming QEMU treats all disk
> paths as local files, and thus not got explicit code to check whether 
> a URI is passed. I could well see that some will consider it a security
> issue to allow QEMU to download off the net, but if they updated to
> a new QEMU with this patch, downloading would be allowed by default.
>   

QEMU already supports protocol URLs (like nbd://).  If a management app 
has some reason to restrict what QEMU has access to, they should be 
using SELinux or already scrubbing device names.  I don't understand why 
accessing a URL would be a security issue though.  I expect that 
management apps should be running QEMU as a non-privileged user and that 
from a security perspective, that user is restricted as much as the 
guest would be restricted.

Regards,

Anthony Liguori