Re: [Qemu-block] [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 <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?

-Jeff