[Qemu-trivial] [PATCH] curl: Operate on zero-length file

[Qemu-trivial] [PATCH] curl: Operate on zero-length file

[Tomáš Golembiovský]

This is an attempt to fix small bug 1596870.

When creating new disk backed by remote file accessed via HTTPS and the
backing file has zero length, qemu-img terminates with uniformative
error message:

    qemu-img: disk.qcow2: CURL: Error opening file:

While it may not make much sense to operate on empty file, other block
backends (e.g. raw backend for regular files) seem to allow it. This
patch fixes it for the curl backend.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 block/curl.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index da9f5e8..ee6c5ea 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -675,11 +675,17 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
     if (curl_easy_perform(state->curl))
         goto out;
-    curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
-    if (d)
-        s->len = (size_t)d;
-    else if(!s->len)
+    if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
         goto out;
+    }
+    if (d < 0) {
+        pstrcpy(state->errmsg, CURL_ERROR_SIZE,
+                "Received invalid file length.");
+        goto out;
+    }
+
+    s->len = (size_t)d;
+
     if ((!strncasecmp(s->url, "http://", strlen("http://"))
         || !strncasecmp(s->url, "https://", strlen("https://")))
         && !s->accept_range) {
-- 
2.9.0

[Qemu-devel] [PATCH] curl: Operate on zero-length file

[Tomáš Golembiovský]

This is an attempt to fix small bug 1596870.

When creating new disk backed by remote file accessed via HTTPS and the
backing file has zero length, qemu-img terminates with uniformative
error message:

    qemu-img: disk.qcow2: CURL: Error opening file:

While it may not make much sense to operate on empty file, other block
backends (e.g. raw backend for regular files) seem to allow it. This
patch fixes it for the curl backend.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 block/curl.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index da9f5e8..ee6c5ea 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -675,11 +675,17 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
     if (curl_easy_perform(state->curl))
         goto out;
-    curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
-    if (d)
-        s->len = (size_t)d;
-    else if(!s->len)
+    if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
         goto out;
+    }
+    if (d < 0) {
+        pstrcpy(state->errmsg, CURL_ERROR_SIZE,
+                "Received invalid file length.");
+        goto out;
+    }
+
+    s->len = (size_t)d;
+
     if ((!strncasecmp(s->url, "http://", strlen("http://"))
         || !strncasecmp(s->url, "https://", strlen("https://")))
         && !s->accept_range) {
-- 
2.9.0

Re: [Qemu-trivial] [Qemu-block] [PATCH] curl: Operate on zero-length file

[Kevin Wolf]

Am 28.06.2016 um 12:37 hat Tomáš Golembiovský geschrieben:
> This is an attempt to fix small bug 1596870.
> 
> When creating new disk backed by remote file accessed via HTTPS and the
> backing file has zero length, qemu-img terminates with uniformative
> error message:
> 
>     qemu-img: disk.qcow2: CURL: Error opening file:
> 
> While it may not make much sense to operate on empty file, other block
> backends (e.g. raw backend for regular files) seem to allow it. This
> patch fixes it for the curl backend.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  block/curl.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index da9f5e8..ee6c5ea 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -675,11 +675,17 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
>      if (curl_easy_perform(state->curl))
>          goto out;
> -    curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
> -    if (d)
> -        s->len = (size_t)d;
> -    else if(!s->len)
> +    if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
>          goto out;
> +    }
> +    if (d < 0) {
> +        pstrcpy(state->errmsg, CURL_ERROR_SIZE,
> +                "Received invalid file length.");
> +        goto out;
> +    }

Actually, it seems that d == -1 means that there is no information about
the file length, so maybe the error message could be a bit more precise.


A possible problem with the patch is that curl changed its behaviour in
version 7.19.4. Before this version, it used to return 0 for a missing
length, so we can't distinguish these cases on older versions. It's
probably more common not to have the file length than having a
zero-length file.

Do we care about older versions? Commit 8a8f584 suggests that we did
three years ago, it added an #if for a feature added in 7.19.4. Not sure
if we do any more. RHEL 6 seems to be new enough.

But if we do care, I guess we could do a similar #if that enables your
code for newer curl versions and keeps erroring out for older versions.

Any opinions on whether we still care about curl versions that old?

> +    s->len = (size_t)d;
> +
>      if ((!strncasecmp(s->url, "http://", strlen("http://"))
>          || !strncasecmp(s->url, "https://", strlen("https://")))
>          && !s->accept_range) {

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH] curl: Operate on zero-length file

[Kevin Wolf]

Am 28.06.2016 um 12:37 hat Tomáš Golembiovský geschrieben:
> This is an attempt to fix small bug 1596870.
> 
> When creating new disk backed by remote file accessed via HTTPS and the
> backing file has zero length, qemu-img terminates with uniformative
> error message:
> 
>     qemu-img: disk.qcow2: CURL: Error opening file:
> 
> While it may not make much sense to operate on empty file, other block
> backends (e.g. raw backend for regular files) seem to allow it. This
> patch fixes it for the curl backend.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  block/curl.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index da9f5e8..ee6c5ea 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -675,11 +675,17 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>      curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
>      if (curl_easy_perform(state->curl))
>          goto out;
> -    curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
> -    if (d)
> -        s->len = (size_t)d;
> -    else if(!s->len)
> +    if (curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d)) {
>          goto out;
> +    }
> +    if (d < 0) {
> +        pstrcpy(state->errmsg, CURL_ERROR_SIZE,
> +                "Received invalid file length.");
> +        goto out;
> +    }

Actually, it seems that d == -1 means that there is no information about
the file length, so maybe the error message could be a bit more precise.


A possible problem with the patch is that curl changed its behaviour in
version 7.19.4. Before this version, it used to return 0 for a missing
length, so we can't distinguish these cases on older versions. It's
probably more common not to have the file length than having a
zero-length file.

Do we care about older versions? Commit 8a8f584 suggests that we did
three years ago, it added an #if for a feature added in 7.19.4. Not sure
if we do any more. RHEL 6 seems to be new enough.

But if we do care, I guess we could do a similar #if that enables your
code for newer curl versions and keeps erroring out for older versions.

Any opinions on whether we still care about curl versions that old?

> +    s->len = (size_t)d;
> +
>      if ((!strncasecmp(s->url, "http://", strlen("http://"))
>          || !strncasecmp(s->url, "https://", strlen("https://")))
>          && !s->accept_range) {

Kevin