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

[Max Reitz]

On 09.11.2016 20:15, Jeff Cody wrote:
> On Tue, Nov 08, 2016 at 08:14:58AM +0100, Markus Armbruster wrote:
>> Max Reitz <address@hidden> writes:
>>
>>> On 07.11.2016 09:20, Markus Armbruster wrote:
>>>> Max Reitz <address@hidden> writes:
>>>>
>>>>> On 03.11.2016 08:56, Markus Armbruster wrote:
>>>>>> Max Reitz <address@hidden> 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

signature.asc
Description: OpenPGP digital signature