[Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Max Reitz]

See patch 3 for the reason why we have actually never supported TFTP at
all (except for very small files (i.e. below 256 kB or so)).

I would consider this series a bug fix because, well, it doesn't really
change any functionality, and the bug is "We don't support TFTP but we
pretend we do".


Alternatives to this approach:

- Deprecate TFTP first. Wait one version, then drop it.

  We could do this, but I personally don't think it's necessary. We have
  done this for host_floppy, but in contrast to host_floppy, TFTP really
  has never worked. Thus, I conclude that nobody is actually using it or
  has ever used it for real work.

  Still, if you think otherwise, we can still do this, of course.


- Don't remove TFTP altogether, but just emit a run-time error like we
  do for HTTP servers that do not support range-based requests.

  Seems dirty and not like the real solution to me. Also, we have
  removed other block drivers in the past, so I don't think we should
  keep TFTP.


Max Reitz (3):
  qemu-options: Drop mentions of curl's TFTP support
  qapi: Drop curl's TFTP protocol
  block/curl: Drop TFTP "support"

 block/curl.c          | 20 +-------------------
 docs/qmp-commands.txt |  2 +-
 qapi/block-core.json  |  7 +++----
 qemu-options.hx       |  6 +++---
 4 files changed, 8 insertions(+), 27 deletions(-)

-- 
2.10.2

[Qemu-devel] [PATCH for-2.8? 1/3] qemu-options: Drop mentions of curl's TFTP support

[Max Reitz]

A follow-up patch will remove the curl block driver's TFTP support;
therefore, we should no longer mention it anywhere in our documentation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-options.hx | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 95332cc..7212779 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2606,8 +2606,8 @@ qemu-system-x86_64 --drive file=gluster://192.0.2.1/testvol/a.img
 
 See also @url{http://www.gluster.org}.
 
-@item HTTP/HTTPS/FTP/FTPS/TFTP
-QEMU supports read-only access to files accessed over http(s), ftp(s) and tftp.
+@item HTTP/HTTPS/FTP/FTPS
+QEMU supports read-only access to files accessed over http(s) and ftp(s).
 
 Syntax using a single filename:
 @example
@@ -2617,7 +2617,7 @@ Syntax using a single filename:
 where:
 @table @option
 @item protocol
-'http', 'https', 'ftp', 'ftps', or 'tftp'.
+'http', 'https', 'ftp', or 'ftps'.
 
 @item username
 Optional username for authentication to the remote server.
-- 
2.10.2

Re: [Qemu-devel] [PATCH for-2.8? 1/3] qemu-options: Drop mentions of curl's TFTP support

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:37PM +0100, Max Reitz wrote:
> A follow-up patch will remove the curl block driver's TFTP support;
> therefore, we should no longer mention it anywhere in our documentation.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-options.hx | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 95332cc..7212779 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2606,8 +2606,8 @@ qemu-system-x86_64 --drive file=gluster://192.0.2.1/testvol/a.img
>  
>  See also @url{http://www.gluster.org}.
>  
> -@item HTTP/HTTPS/FTP/FTPS/TFTP
> -QEMU supports read-only access to files accessed over http(s), ftp(s) and tftp.
> +@item HTTP/HTTPS/FTP/FTPS
> +QEMU supports read-only access to files accessed over http(s) and ftp(s).
>  
>  Syntax using a single filename:
>  @example
> @@ -2617,7 +2617,7 @@ Syntax using a single filename:
>  where:
>  @table @option
>  @item protocol
> -'http', 'https', 'ftp', 'ftps', or 'tftp'.
> +'http', 'https', 'ftp', or 'ftps'.
>  
>  @item username
>  Optional username for authentication to the remote server.
> -- 
> 2.10.2
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH for-2.8? 2/3] qapi: Drop curl's TFTP protocol

[Max Reitz]

A follow-up patch will remove the curl block driver's TFTP support, so
remove the protocol from the QAPI schema.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 docs/qmp-commands.txt | 2 +-
 qapi/block-core.json  | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index 6afa872..abf210a 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -1803,7 +1803,7 @@ Each json-object contain the following:
                                 "file", "file", "ftp", "ftps", "host_cdrom",
                                 "host_device", "http", "https",
                                 "nbd", "parallels", "qcow", "qcow2", "raw",
-                                "tftp", "vdi", "vmdk", "vpc", "vvfat"
+                                "vdi", "vmdk", "vpc", "vvfat"
          - "backing_file": backing file name (json-string, optional)
          - "backing_file_depth": number of files in the backing file chain (json-int)
          - "encrypted": true if encrypted, false otherwise (json-bool)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bcd3b9e..c29bef7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -243,12 +243,12 @@
 #       0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
 #       'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
 #       'http', 'https', 'luks', 'nbd', 'parallels', 'qcow',
-#       'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat'
+#       'qcow2', 'raw', 'vdi', 'vmdk', 'vpc', 'vvfat'
 #       2.2: 'archipelago' added, 'cow' dropped
 #       2.3: 'host_floppy' deprecated
 #       2.5: 'host_floppy' dropped
 #       2.6: 'luks' added
-#       2.8: 'replication' added
+#       2.8: 'replication' added, 'tftp' dropped
 #
 # @backing_file: #optional the name of the backing file (for copy-on-write)
 #
@@ -1723,7 +1723,7 @@
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
             'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
             'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-            'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
+            'replication', 'ssh', 'vdi', 'vhdx', 'vmdk', 'vpc',
             'vvfat' ] }
 
 ##
@@ -2410,7 +2410,6 @@
       'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
       'ssh':        'BlockdevOptionsSsh',
-      'tftp':       'BlockdevOptionsCurl',
       'vdi':        'BlockdevOptionsGenericFormat',
       'vhdx':       'BlockdevOptionsGenericFormat',
       'vmdk':       'BlockdevOptionsGenericCOWFormat',
-- 
2.10.2

Re: [Qemu-devel] [PATCH for-2.8? 2/3] qapi: Drop curl's TFTP protocol

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:38PM +0100, Max Reitz wrote:
> A follow-up patch will remove the curl block driver's TFTP support, so
> remove the protocol from the QAPI schema.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  docs/qmp-commands.txt | 2 +-
>  qapi/block-core.json  | 7 +++----
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> index 6afa872..abf210a 100644
> --- a/docs/qmp-commands.txt
> +++ b/docs/qmp-commands.txt
> @@ -1803,7 +1803,7 @@ Each json-object contain the following:
>                                  "file", "file", "ftp", "ftps", "host_cdrom",
>                                  "host_device", "http", "https",
>                                  "nbd", "parallels", "qcow", "qcow2", "raw",
> -                                "tftp", "vdi", "vmdk", "vpc", "vvfat"
> +                                "vdi", "vmdk", "vpc", "vvfat"
>           - "backing_file": backing file name (json-string, optional)
>           - "backing_file_depth": number of files in the backing file chain (json-int)
>           - "encrypted": true if encrypted, false otherwise (json-bool)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index bcd3b9e..c29bef7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -243,12 +243,12 @@
>  #       0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
>  #       'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
>  #       'http', 'https', 'luks', 'nbd', 'parallels', 'qcow',
> -#       'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat'
> +#       'qcow2', 'raw', 'vdi', 'vmdk', 'vpc', 'vvfat'
>  #       2.2: 'archipelago' added, 'cow' dropped
>  #       2.3: 'host_floppy' deprecated
>  #       2.5: 'host_floppy' dropped
>  #       2.6: 'luks' added
> -#       2.8: 'replication' added
> +#       2.8: 'replication' added, 'tftp' dropped
>  #
>  # @backing_file: #optional the name of the backing file (for copy-on-write)
>  #
> @@ -1723,7 +1723,7 @@
>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>              'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
>              'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> -            'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
> +            'replication', 'ssh', 'vdi', 'vhdx', 'vmdk', 'vpc',
>              'vvfat' ] }
>  
>  ##
> @@ -2410,7 +2410,6 @@
>        'replication':'BlockdevOptionsReplication',
>  # TODO sheepdog: Wait for structured options
>        'ssh':        'BlockdevOptionsSsh',
> -      'tftp':       'BlockdevOptionsCurl',
>        'vdi':        'BlockdevOptionsGenericFormat',
>        'vhdx':       'BlockdevOptionsGenericFormat',
>        'vmdk':       'BlockdevOptionsGenericCOWFormat',
> -- 
> 2.10.2
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH for-2.8? 3/3] block/curl: Drop TFTP "support"

[Max Reitz]

Because TFTP does not support byte ranges, it was never usable with our
curl block driver. Since apparently nobody has ever complained loudly
enough for someone to take care of the issue until now, it seems
reasonable to assume that nobody has ever actually used it.

Therefore, it should be safe to just drop it from curl's protocol list.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index e5eaa7b..ba8adae 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -68,8 +68,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
 #endif
 
 #define PROTOCOLS (CURLPROTO_HTTP | CURLPROTO_HTTPS | \
-                   CURLPROTO_FTP | CURLPROTO_FTPS | \
-                   CURLPROTO_TFTP)
+                   CURLPROTO_FTP | CURLPROTO_FTPS)
 
 #define CURL_NUM_STATES 8
 #define CURL_NUM_ACB    8
@@ -886,29 +885,12 @@ static BlockDriver bdrv_ftps = {
     .bdrv_attach_aio_context    = curl_attach_aio_context,
 };
 
-static BlockDriver bdrv_tftp = {
-    .format_name                = "tftp",
-    .protocol_name              = "tftp",
-
-    .instance_size              = sizeof(BDRVCURLState),
-    .bdrv_parse_filename        = curl_parse_filename,
-    .bdrv_file_open             = curl_open,
-    .bdrv_close                 = curl_close,
-    .bdrv_getlength             = curl_getlength,
-
-    .bdrv_aio_readv             = curl_aio_readv,
-
-    .bdrv_detach_aio_context    = curl_detach_aio_context,
-    .bdrv_attach_aio_context    = curl_attach_aio_context,
-};
-
 static void curl_block_init(void)
 {
     bdrv_register(&bdrv_http);
     bdrv_register(&bdrv_https);
     bdrv_register(&bdrv_ftp);
     bdrv_register(&bdrv_ftps);
-    bdrv_register(&bdrv_tftp);
 }
 
 block_init(curl_block_init);
-- 
2.10.2

Re: [Qemu-devel] [PATCH for-2.8? 3/3] block/curl: Drop TFTP "support"

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:39PM +0100, Max Reitz wrote:
> Because TFTP does not support byte ranges, it was never usable with our
> curl block driver. Since apparently nobody has ever complained loudly
> enough for someone to take care of the issue until now, it seems
> reasonable to assume that nobody has ever actually used it.
> 
> Therefore, it should be safe to just drop it from curl's protocol list.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index e5eaa7b..ba8adae 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -68,8 +68,7 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
>  #endif
>  
>  #define PROTOCOLS (CURLPROTO_HTTP | CURLPROTO_HTTPS | \
> -                   CURLPROTO_FTP | CURLPROTO_FTPS | \
> -                   CURLPROTO_TFTP)
> +                   CURLPROTO_FTP | CURLPROTO_FTPS)
>  
>  #define CURL_NUM_STATES 8
>  #define CURL_NUM_ACB    8
> @@ -886,29 +885,12 @@ static BlockDriver bdrv_ftps = {
>      .bdrv_attach_aio_context    = curl_attach_aio_context,
>  };
>  
> -static BlockDriver bdrv_tftp = {
> -    .format_name                = "tftp",
> -    .protocol_name              = "tftp",
> -
> -    .instance_size              = sizeof(BDRVCURLState),
> -    .bdrv_parse_filename        = curl_parse_filename,
> -    .bdrv_file_open             = curl_open,
> -    .bdrv_close                 = curl_close,
> -    .bdrv_getlength             = curl_getlength,
> -
> -    .bdrv_aio_readv             = curl_aio_readv,
> -
> -    .bdrv_detach_aio_context    = curl_detach_aio_context,
> -    .bdrv_attach_aio_context    = curl_attach_aio_context,
> -};
> -
>  static void curl_block_init(void)
>  {
>      bdrv_register(&bdrv_http);
>      bdrv_register(&bdrv_https);
>      bdrv_register(&bdrv_ftp);
>      bdrv_register(&bdrv_ftps);
> -    bdrv_register(&bdrv_tftp);
>  }
>  
>  block_init(curl_block_init);
> -- 
> 2.10.2
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:36PM +0100, Max Reitz wrote:
> See patch 3 for the reason why we have actually never supported TFTP at
> all (except for very small files (i.e. below 256 kB or so)).
> 
> I would consider this series a bug fix because, well, it doesn't really
> change any functionality, and the bug is "We don't support TFTP but we
> pretend we do".
> 

I tend to agree.  I'm willing to pull this in through my branch for 2.8,
unless there arises some outcry with good reason to keep tftp.

> 
> Alternatives to this approach:
> 
> - Deprecate TFTP first. Wait one version, then drop it.
> 
>   We could do this, but I personally don't think it's necessary. We have
>   done this for host_floppy, but in contrast to host_floppy, TFTP really
>   has never worked. Thus, I conclude that nobody is actually using it or
>   has ever used it for real work.
> 
>   Still, if you think otherwise, we can still do this, of course.
> 
> 
> - Don't remove TFTP altogether, but just emit a run-time error like we
>   do for HTTP servers that do not support range-based requests.
> 
>   Seems dirty and not like the real solution to me. Also, we have
>   removed other block drivers in the past, so I don't think we should
>   keep TFTP.
> 

Since it is broken by nature, I like your original approach of just removing
it.

> 
> Max Reitz (3):
>   qemu-options: Drop mentions of curl's TFTP support
>   qapi: Drop curl's TFTP protocol
>   block/curl: Drop TFTP "support"
> 
>  block/curl.c          | 20 +-------------------
>  docs/qmp-commands.txt |  2 +-
>  qapi/block-core.json  |  7 +++----
>  qemu-options.hx       |  6 +++---
>  4 files changed, 8 insertions(+), 27 deletions(-)

A > 3:1 delete to insert ratio, that is an ideal patch series ;-)

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> See patch 3 for the reason why we have actually never supported TFTP at
> all (except for very small files (i.e. below 256 kB or so)).

Care to explain why it works "for very small files" in a bit more
detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
from there to "very small files", you need to know more about how the
block layer works than I can remember right now.

> I would consider this series a bug fix because, well, it doesn't really
> change any functionality, and the bug is "We don't support TFTP but we
> pretend we do".
>
>
> Alternatives to this approach:
>
> - Deprecate TFTP first. Wait one version, then drop it.
>
>   We could do this, but I personally don't think it's necessary. We have
>   done this for host_floppy, but in contrast to host_floppy, TFTP really
>   has never worked.

"(except for very small files (i.e. below 256 kB or so)"

>                     Thus, I conclude that nobody is actually using it or
>   has ever used it for real work.

Plausible.

>   Still, if you think otherwise, we can still do this, of course.
>
> - Don't remove TFTP altogether, but just emit a run-time error like we
>   do for HTTP servers that do not support range-based requests.
>
>   Seems dirty and not like the real solution to me. Also, we have
>   removed other block drivers in the past, so I don't think we should
>   keep TFTP.

Well, the run-time error with HTTP happens when we have a losing server,
while with TFTP, no non-losing servers can exist (except for very small
files).

Anyway, I'm fine with dropping TFTP right away.  I'd probably squash the
three patches, but that's a matter of taste.

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On 03.11.2016 08:56, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> See patch 3 for the reason why we have actually never supported TFTP at
>> all (except for very small files (i.e. below 256 kB or so)).
> 
> Care to explain why it works "for very small files" in a bit more
> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
> from there to "very small files", you need to know more about how the
> block layer works than I can remember right now.

Our curl block drivers caches data and uses a readahead cache, which by
default has a size of 256 kB. Therefore, if the start of the file is
read first (which it usually is, if just for format probing), then the
correct data will be read for that size.

Yes, you can adjust the readahead size. No, I cannot guarantee that
there are no users that just set readahead to the image size and thus
made it work. I can't really imagine that, though, because at that point
you can just copy the file to tmpfs and have the same result.

Also, if I were a user, I probably wouldn't use 256 kB images, and thus
I would just notice tftp to be broken. I don't think I would experiment
with the readahead option to find out that it works if I set it to the
image size and then just use it that way. I definitely think I would
give up before that and just copy the file to the local system.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 03.11.2016 08:56, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> See patch 3 for the reason why we have actually never supported TFTP at
>>> all (except for very small files (i.e. below 256 kB or so)).
>> 
>> Care to explain why it works "for very small files" in a bit more
>> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
>> from there to "very small files", you need to know more about how the
>> block layer works than I can remember right now.
>
> Our curl block drivers caches data and uses a readahead cache, which by
> default has a size of 256 kB. Therefore, if the start of the file is
> read first (which it usually is, if just for format probing), then the
> correct data will be read for that size.
>
> Yes, you can adjust the readahead size. No, I cannot guarantee that
> there are no users that just set readahead to the image size and thus
> made it work. I can't really imagine that, though, because at that point
> you can just copy the file to tmpfs and have the same result.
>
> Also, if I were a user, I probably wouldn't use 256 kB images, and thus
> I would just notice tftp to be broken. I don't think I would experiment
> with the readahead option to find out that it works if I set it to the
> image size and then just use it that way. I definitely think I would
> give up before that and just copy the file to the local system.

I'm not trying to make you explain why it's okay to drop TFTP.  I'm
trying to make you explain what exactly worked and what exactly didn't.
Such explanations generally involve a certain degree of "why".

Your first paragraph provides a few more hints, but I'm still guessing.
Here's my current best guess:

* Commonly, images smaller than 256 KiB work, and larger images don't.

* "Don't work" means the block layer returns garbled data.

* "Commonly" means when the first read is for offset zero.  Begs the
  question when exactly that's the case.  You mentioned format probing.
  What if the user specified a format?  It's okay not to answer this
  question.  I'm not demanding exhaustive analysis, I'm fishing for a
  better commit message.  Such a message may leave some of its questions
  unanswered.

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3936 bytes --]

On 07.11.2016 09:20, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 03.11.2016 08:56, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> See patch 3 for the reason why we have actually never supported TFTP at
>>>> all (except for very small files (i.e. below 256 kB or so)).
>>>
>>> Care to explain why it works "for very small files" in a bit more
>>> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
>>> from there to "very small files", you need to know more about how the
>>> block layer works than I can remember right now.
>>
>> Our curl block drivers caches data and uses a readahead cache, which by
>> default has a size of 256 kB. Therefore, if the start of the file is
>> read first (which it usually is, if just for format probing), then the
>> correct data will be read for that size.
>>
>> Yes, you can adjust the readahead size. No, I cannot guarantee that
>> there are no users that just set readahead to the image size and thus
>> made it work. I can't really imagine that, though, because at that point
>> you can just copy the file to tmpfs and have the same result.
>>
>> Also, if I were a user, I probably wouldn't use 256 kB images, and thus
>> I would just notice tftp to be broken. I don't think I would experiment
>> with the readahead option to find out that it works if I set it to the
>> image size and then just use it that way. I definitely think I would
>> give up before that and just copy the file to the local system.
> 
> I'm not trying to make you explain why it's okay to drop TFTP.  I'm
> trying to make you explain what exactly worked and what exactly didn't.
> Such explanations generally involve a certain degree of "why".

Well, I'm trying to explain both. :-)

> Your first paragraph provides a few more hints, but I'm still guessing.
> Here's my current best guess:
> 
> * Commonly, images smaller than 256 KiB work, and larger images don't.

Yes. Unless you set the "readahead" option to something different (it
just defaults to 256 kB), then it'll commonly work for that images up to
that size.

Oh, and I just realized it's not called "readahead" for nothing: It gets
added to the size of the read operation, so if your first read operation
has a size of 1 GB... Well, then all of that will be correctly cached.
So both the size and the offset of the first read operation are significant.

> * "Don't work" means the block layer returns garbled data.

Right. It will be data from the image, but not from the offset you want.

> * "Commonly" means when the first read is for offset zero.  Begs the
>   question when exactly that's the case.  You mentioned format probing.
>   What if the user specified a format?  It's okay not to answer this
>   question.  I'm not demanding exhaustive analysis, I'm fishing for a
>   better commit message.  Such a message may leave some of its questions
>   unanswered.

Well, qcow2 will always start at offset zero anyway (because it reads
the header first). For raw images, the offset can be anywhere, but if
you're starting a VM from it, offset zero is obviously likely to be read
first, too.

(And as a side note, the first read operation for qcow2 images will
always be 64 kB in size.)

But, yes, for raw images the offset can be anywhere and if it is not
zero, the answer what works and what doesn't becomes a bit more complicated:

<optional>
Suppose the first offset read from is 64k. curl will return data from
offset 0 anyway, so it's pretty much garbage. But if you then do another
read operation from 0, that will return correct data.

If after that you try to read data from the area that has been covered
by both read operations... Then it depends on which buffer the curl
driver sees first, which is most likely the first one, i.e. you'll get
broken data again.
</optional>

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 07.11.2016 09:20, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 03.11.2016 08:56, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> See patch 3 for the reason why we have actually never supported TFTP at
>>>>> all (except for very small files (i.e. below 256 kB or so)).
>>>>
>>>> Care to explain why it works "for very small files" in a bit more
>>>> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
>>>> from there to "very small files", you need to know more about how the
>>>> block layer works than I can remember right now.
>>>
>>> Our curl block drivers caches data and uses a readahead cache, which by
>>> default has a size of 256 kB. Therefore, if the start of the file is
>>> read first (which it usually is, if just for format probing), then the
>>> correct data will be read for that size.
>>>
>>> Yes, you can adjust the readahead size. No, I cannot guarantee that
>>> there are no users that just set readahead to the image size and thus
>>> made it work. I can't really imagine that, though, because at that point
>>> you can just copy the file to tmpfs and have the same result.
>>>
>>> Also, if I were a user, I probably wouldn't use 256 kB images, and thus
>>> I would just notice tftp to be broken. I don't think I would experiment
>>> with the readahead option to find out that it works if I set it to the
>>> image size and then just use it that way. I definitely think I would
>>> give up before that and just copy the file to the local system.
>> 
>> I'm not trying to make you explain why it's okay to drop TFTP.  I'm
>> trying to make you explain what exactly worked and what exactly didn't.
>> Such explanations generally involve a certain degree of "why".
>
> Well, I'm trying to explain both. :-)
>
>> Your first paragraph provides a few more hints, but I'm still guessing.
>> Here's my current best guess:
>> 
>> * Commonly, images smaller than 256 KiB work, and larger images don't.
>
> Yes. Unless you set the "readahead" option to something different (it
> just defaults to 256 kB), then it'll commonly work for that images up to
> that size.
>
> Oh, and I just realized it's not called "readahead" for nothing: It gets
> added to the size of the read operation, so if your first read operation
> has a size of 1 GB... Well, then all of that will be correctly cached.
> So both the size and the offset of the first read operation are significant.
>
>> * "Don't work" means the block layer returns garbled data.
>
> Right. It will be data from the image, but not from the offset you want.
>
>> * "Commonly" means when the first read is for offset zero.  Begs the
>>   question when exactly that's the case.  You mentioned format probing.
>>   What if the user specified a format?  It's okay not to answer this
>>   question.  I'm not demanding exhaustive analysis, I'm fishing for a
>>   better commit message.  Such a message may leave some of its questions
>>   unanswered.
>
> Well, qcow2 will always start at offset zero anyway (because it reads
> the header first). For raw images, the offset can be anywhere, but if
> you're starting a VM from it, offset zero is obviously likely to be read
> first, too.
>
> (And as a side note, the first read operation for qcow2 images will
> always be 64 kB in size.)
>
> But, yes, for raw images the offset can be anywhere and if it is not
> zero, the answer what works and what doesn't becomes a bit more complicated:
>
> <optional>
> Suppose the first offset read from is 64k. curl will return data from
> offset 0 anyway, so it's pretty much garbage. But if you then do another
> read operation from 0, that will return correct data.
>
> If after that you try to read data from the area that has been covered
> by both read operations... Then it depends on which buffer the curl
> driver sees first, which is most likely the first one, i.e. you'll get
> broken data again.
> </optional>

There's a lovely addition to your commit message struggling to get out
of your reply.

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Jeff Cody]

On Tue, Nov 08, 2016 at 08:14:58AM +0100, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
> > On 07.11.2016 09:20, Markus Armbruster wrote:
> >> Max Reitz <mreitz@redhat.com> writes:
> >> 
> >>> On 03.11.2016 08:56, Markus Armbruster wrote:
> >>>> Max Reitz <mreitz@redhat.com> writes:
> >>>>
> >>>>> See patch 3 for the reason why we have actually never supported TFTP at
> >>>>> all (except for very small files (i.e. below 256 kB or so)).
> >>>>
> >>>> Care to explain why it works "for very small files" in a bit more
> >>>> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
> >>>> from there to "very small files", you need to know more about how the
> >>>> block layer works than I can remember right now.
> >>>
> >>> Our curl block drivers caches data and uses a readahead cache, which by
> >>> default has a size of 256 kB. Therefore, if the start of the file is
> >>> read first (which it usually is, if just for format probing), then the
> >>> correct data will be read for that size.
> >>>
> >>> Yes, you can adjust the readahead size. No, I cannot guarantee that
> >>> there are no users that just set readahead to the image size and thus
> >>> made it work. I can't really imagine that, though, because at that point
> >>> you can just copy the file to tmpfs and have the same result.
> >>>
> >>> Also, if I were a user, I probably wouldn't use 256 kB images, and thus
> >>> I would just notice tftp to be broken. I don't think I would experiment
> >>> with the readahead option to find out that it works if I set it to the
> >>> image size and then just use it that way. I definitely think I would
> >>> give up before that and just copy the file to the local system.
> >> 
> >> I'm not trying to make you explain why it's okay to drop TFTP.  I'm
> >> trying to make you explain what exactly worked and what exactly didn't.
> >> Such explanations generally involve a certain degree of "why".
> >
> > Well, I'm trying to explain both. :-)
> >
> >> Your first paragraph provides a few more hints, but I'm still guessing.
> >> Here's my current best guess:
> >> 
> >> * Commonly, images smaller than 256 KiB work, and larger images don't.
> >
> > Yes. Unless you set the "readahead" option to something different (it
> > just defaults to 256 kB), then it'll commonly work for that images up to
> > that size.
> >
> > Oh, and I just realized it's not called "readahead" for nothing: It gets
> > added to the size of the read operation, so if your first read operation
> > has a size of 1 GB... Well, then all of that will be correctly cached.
> > So both the size and the offset of the first read operation are significant.
> >
> >> * "Don't work" means the block layer returns garbled data.
> >
> > Right. It will be data from the image, but not from the offset you want.
> >
> >> * "Commonly" means when the first read is for offset zero.  Begs the
> >>   question when exactly that's the case.  You mentioned format probing.
> >>   What if the user specified a format?  It's okay not to answer this
> >>   question.  I'm not demanding exhaustive analysis, I'm fishing for a
> >>   better commit message.  Such a message may leave some of its questions
> >>   unanswered.
> >
> > Well, qcow2 will always start at offset zero anyway (because it reads
> > the header first). For raw images, the offset can be anywhere, but if
> > you're starting a VM from it, offset zero is obviously likely to be read
> > first, too.
> >
> > (And as a side note, the first read operation for qcow2 images will
> > always be 64 kB in size.)
> >
> > But, yes, for raw images the offset can be anywhere and if it is not
> > zero, the answer what works and what doesn't becomes a bit more complicated:
> >
> > <optional>
> > Suppose the first offset read from is 64k. curl will return data from
> > offset 0 anyway, so it's pretty much garbage. But if you then do another
> > read operation from 0, that will return correct data.
> >
> > If after that you try to read data from the area that has been covered
> > by both read operations... Then it depends on which buffer the curl
> > driver sees first, which is most likely the first one, i.e. you'll get
> > broken data again.
> > </optional>
> 
> There's a lovely addition to your commit message struggling to get out
> of your reply.

I'm going to go ahead and apply the series; I think the relevant point
for the commit message is that TFTP is not usable and never has been.  If
Max has no objections, I'll pull some wording in from his reply here into
his commit message for patch 3, and squash all the patches into one.  

Max, any objections?

-Jeff

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4845 bytes --]

On 09.11.2016 20:15, Jeff Cody wrote:
> On Tue, Nov 08, 2016 at 08:14:58AM +0100, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>>
>>> On 07.11.2016 09:20, Markus Armbruster wrote:
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> On 03.11.2016 08:56, Markus Armbruster wrote:
>>>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>>>
>>>>>>> See patch 3 for the reason why we have actually never supported TFTP at
>>>>>>> all (except for very small files (i.e. below 256 kB or so)).
>>>>>>
>>>>>> Care to explain why it works "for very small files" in a bit more
>>>>>> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
>>>>>> from there to "very small files", you need to know more about how the
>>>>>> block layer works than I can remember right now.
>>>>>
>>>>> Our curl block drivers caches data and uses a readahead cache, which by
>>>>> default has a size of 256 kB. Therefore, if the start of the file is
>>>>> read first (which it usually is, if just for format probing), then the
>>>>> correct data will be read for that size.
>>>>>
>>>>> Yes, you can adjust the readahead size. No, I cannot guarantee that
>>>>> there are no users that just set readahead to the image size and thus
>>>>> made it work. I can't really imagine that, though, because at that point
>>>>> you can just copy the file to tmpfs and have the same result.
>>>>>
>>>>> Also, if I were a user, I probably wouldn't use 256 kB images, and thus
>>>>> I would just notice tftp to be broken. I don't think I would experiment
>>>>> with the readahead option to find out that it works if I set it to the
>>>>> image size and then just use it that way. I definitely think I would
>>>>> give up before that and just copy the file to the local system.
>>>>
>>>> I'm not trying to make you explain why it's okay to drop TFTP.  I'm
>>>> trying to make you explain what exactly worked and what exactly didn't.
>>>> Such explanations generally involve a certain degree of "why".
>>>
>>> Well, I'm trying to explain both. :-)
>>>
>>>> Your first paragraph provides a few more hints, but I'm still guessing.
>>>> Here's my current best guess:
>>>>
>>>> * Commonly, images smaller than 256 KiB work, and larger images don't.
>>>
>>> Yes. Unless you set the "readahead" option to something different (it
>>> just defaults to 256 kB), then it'll commonly work for that images up to
>>> that size.
>>>
>>> Oh, and I just realized it's not called "readahead" for nothing: It gets
>>> added to the size of the read operation, so if your first read operation
>>> has a size of 1 GB... Well, then all of that will be correctly cached.
>>> So both the size and the offset of the first read operation are significant.
>>>
>>>> * "Don't work" means the block layer returns garbled data.
>>>
>>> Right. It will be data from the image, but not from the offset you want.
>>>
>>>> * "Commonly" means when the first read is for offset zero.  Begs the
>>>>   question when exactly that's the case.  You mentioned format probing.
>>>>   What if the user specified a format?  It's okay not to answer this
>>>>   question.  I'm not demanding exhaustive analysis, I'm fishing for a
>>>>   better commit message.  Such a message may leave some of its questions
>>>>   unanswered.
>>>
>>> Well, qcow2 will always start at offset zero anyway (because it reads
>>> the header first). For raw images, the offset can be anywhere, but if
>>> you're starting a VM from it, offset zero is obviously likely to be read
>>> first, too.
>>>
>>> (And as a side note, the first read operation for qcow2 images will
>>> always be 64 kB in size.)
>>>
>>> But, yes, for raw images the offset can be anywhere and if it is not
>>> zero, the answer what works and what doesn't becomes a bit more complicated:
>>>
>>> <optional>
>>> Suppose the first offset read from is 64k. curl will return data from
>>> offset 0 anyway, so it's pretty much garbage. But if you then do another
>>> read operation from 0, that will return correct data.
>>>
>>> If after that you try to read data from the area that has been covered
>>> by both read operations... Then it depends on which buffer the curl
>>> driver sees first, which is most likely the first one, i.e. you'll get
>>> broken data again.
>>> </optional>
>>
>> There's a lovely addition to your commit message struggling to get out
>> of your reply.
> 
> I'm going to go ahead and apply the series; I think the relevant point
> for the commit message is that TFTP is not usable and never has been.  If
> Max has no objections, I'll pull some wording in from his reply here into
> his commit message for patch 3, and squash all the patches into one.  
> 
> Max, any objections?

No, that sounds good to me. Thank you very much!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Jeff Cody]

On Fri, Nov 11, 2016 at 08:46:11PM +0100, Max Reitz wrote:
> On 09.11.2016 20:15, Jeff Cody wrote:
> > On Tue, Nov 08, 2016 at 08:14:58AM +0100, Markus Armbruster wrote:
> >> Max Reitz <mreitz@redhat.com> writes:
> >>
> >>> On 07.11.2016 09:20, Markus Armbruster wrote:
> >>>> Max Reitz <mreitz@redhat.com> writes:
> >>>>
> >>>>> On 03.11.2016 08:56, Markus Armbruster wrote:
> >>>>>> Max Reitz <mreitz@redhat.com> writes:
> >>>>>>
> >>>>>>> See patch 3 for the reason why we have actually never supported TFTP at
> >>>>>>> all (except for very small files (i.e. below 256 kB or so)).
> >>>>>>
> >>>>>> Care to explain why it works "for very small files" in a bit more
> >>>>>> detail?  PATCH 3 gives a "does not support byte ranges" hint, but to go
> >>>>>> from there to "very small files", you need to know more about how the
> >>>>>> block layer works than I can remember right now.
> >>>>>
> >>>>> Our curl block drivers caches data and uses a readahead cache, which by
> >>>>> default has a size of 256 kB. Therefore, if the start of the file is
> >>>>> read first (which it usually is, if just for format probing), then the
> >>>>> correct data will be read for that size.
> >>>>>
> >>>>> Yes, you can adjust the readahead size. No, I cannot guarantee that
> >>>>> there are no users that just set readahead to the image size and thus
> >>>>> made it work. I can't really imagine that, though, because at that point
> >>>>> you can just copy the file to tmpfs and have the same result.
> >>>>>
> >>>>> Also, if I were a user, I probably wouldn't use 256 kB images, and thus
> >>>>> I would just notice tftp to be broken. I don't think I would experiment
> >>>>> with the readahead option to find out that it works if I set it to the
> >>>>> image size and then just use it that way. I definitely think I would
> >>>>> give up before that and just copy the file to the local system.
> >>>>
> >>>> I'm not trying to make you explain why it's okay to drop TFTP.  I'm
> >>>> trying to make you explain what exactly worked and what exactly didn't.
> >>>> Such explanations generally involve a certain degree of "why".
> >>>
> >>> Well, I'm trying to explain both. :-)
> >>>
> >>>> Your first paragraph provides a few more hints, but I'm still guessing.
> >>>> Here's my current best guess:
> >>>>
> >>>> * Commonly, images smaller than 256 KiB work, and larger images don't.
> >>>
> >>> Yes. Unless you set the "readahead" option to something different (it
> >>> just defaults to 256 kB), then it'll commonly work for that images up to
> >>> that size.
> >>>
> >>> Oh, and I just realized it's not called "readahead" for nothing: It gets
> >>> added to the size of the read operation, so if your first read operation
> >>> has a size of 1 GB... Well, then all of that will be correctly cached.
> >>> So both the size and the offset of the first read operation are significant.
> >>>
> >>>> * "Don't work" means the block layer returns garbled data.
> >>>
> >>> Right. It will be data from the image, but not from the offset you want.
> >>>
> >>>> * "Commonly" means when the first read is for offset zero.  Begs the
> >>>>   question when exactly that's the case.  You mentioned format probing.
> >>>>   What if the user specified a format?  It's okay not to answer this
> >>>>   question.  I'm not demanding exhaustive analysis, I'm fishing for a
> >>>>   better commit message.  Such a message may leave some of its questions
> >>>>   unanswered.
> >>>
> >>> Well, qcow2 will always start at offset zero anyway (because it reads
> >>> the header first). For raw images, the offset can be anywhere, but if
> >>> you're starting a VM from it, offset zero is obviously likely to be read
> >>> first, too.
> >>>
> >>> (And as a side note, the first read operation for qcow2 images will
> >>> always be 64 kB in size.)
> >>>
> >>> But, yes, for raw images the offset can be anywhere and if it is not
> >>> zero, the answer what works and what doesn't becomes a bit more complicated:
> >>>
> >>> <optional>
> >>> Suppose the first offset read from is 64k. curl will return data from
> >>> offset 0 anyway, so it's pretty much garbage. But if you then do another
> >>> read operation from 0, that will return correct data.
> >>>
> >>> If after that you try to read data from the area that has been covered
> >>> by both read operations... Then it depends on which buffer the curl
> >>> driver sees first, which is most likely the first one, i.e. you'll get
> >>> broken data again.
> >>> </optional>
> >>
> >> There's a lovely addition to your commit message struggling to get out
> >> of your reply.
> > 
> > I'm going to go ahead and apply the series; I think the relevant point
> > for the commit message is that TFTP is not usable and never has been.  If
> > Max has no objections, I'll pull some wording in from his reply here into
> > his commit message for patch 3, and squash all the patches into one.  
> > 
> > Max, any objections?
> 
> No, that sounds good to me. Thank you very much!
> 
> Max
> 




Thanks,

Applied to my block branch (squashed, and commit message amended):

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

Re: [Qemu-devel] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Kevin Wolf]

Am 02.11.2016 um 18:55 hat Max Reitz geschrieben:
> See patch 3 for the reason why we have actually never supported TFTP at
> all (except for very small files (i.e. below 256 kB or so)).
> 
> I would consider this series a bug fix because, well, it doesn't really
> change any functionality, and the bug is "We don't support TFTP but we
> pretend we do".

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

libvirt added support for tftp in commit 8ffe1d0c, in a series that
added https and ftps, too. I suppose tftp wasn't actually tested. Maybe
libvirt wants to remove tftp as well.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Wed, Nov 02, 2016 at 06:55:36PM +0100, Max Reitz wrote:
> See patch 3 for the reason why we have actually never supported TFTP at
> all (except for very small files (i.e. below 256 kB or so)).

I agree with dropping TFTP from the curl block driver but for the
record:

TFTP is not good for large files anyway - performance is poor due to the
lock-step ACK protocol.  It is used for <256 KB files to chainload
Network Boot Programs (PXE) so the small file scenario is actually
realistic for TFTP.

In that use case it doesn't really make sense to use a block device in
QEMU though.  The guest would do the TFTP itself so I don't see a need
to offer the protocol in the curl block driver.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]