Re: [Qemu-block] null-co undefined read behavior

[Kevin Wolf]

Am 14.06.2017 um 19:44 hat John Snow geschrieben:
> 
> 
> On 06/13/2017 03:52 AM, Kevin Wolf wrote:
> > Am 12.06.2017 um 23:20 hat John Snow geschrieben:
> >> I noticed while debugging an unrelated test that our use of the null
> >> driver has a habit of making functions like find_image_format trigger a
> >> lot of uninitialized memory errors in valgrind, because it will return a
> >> successful read without actually touching the buffer.
> >>
> >> I see that in March there was a bit of a debate over whether or not the
> >> null driver SHOULD zero-write memory for performance reasons, but when
> >> this option isn't specified, the semantics of read are arguably broken.
> >>
> >> This isn't terribly important to fix, but for actual debugging purposes
> >> it's nice to not have valgrind screaming at you with spurious junk. A
> >> few options:
> >>
> >> (1) Admit that it's weird to allow reads to succeed with null-co driver.
> >> Annoy people who apparently wanted performance for their do-nothing
> >> driver and force the initialization of memory.
> >>
> >> --or--
> >>
> >> (1a) Use valgrind macros to pretend the memory has been initialized.
> >>
> >> (2) Band-aid find_image_format to zero-initialize memory. Wonder if
> >> there are other usages of read() getting called from tests that utilize
> >> null-co that will make debugging difficult in other contexts.
> >>
> >> (3) Edit any tests to always use the zero option when using the null
> >> driver, and then periodically keep updating this option in a losing
> >> battle over time.
> >>
> >> (4) Find a way to adjust return value of null-co's read implementation
> >> to return 0 instead of a lie over the number of bytes it read/wrote.
> >> Correctly represents "No bytes written, but no error occurred either."
> > 
> > The block layer doesn't support short reads. Therefore, the return value
> > is 0/-errno even today, the byte count isn't returned anywhere.
> > 
> 
> Run the blockjob transaction test and take a look at what the read is
> returning in find_image_format. It's 512! I didn't look further than
> that, I assumed it was intentional.

Yes, the bdrv_pread/pwrite() functions have always returned byte counts
and still do. They always return the full byte count or -errno, and this
is fully contained in higher level wrappers.

> >> (5) Shut up, John, really, who cares? Please shut up. You are the only
> >> idiot who would even think to use valgrind ever, and we just don't like
> >> you that much.
> > 
> > (6) Make read-zeroes=on the default. Gives people worse performance if
> > they expect that null-co:// without any options is the fastest thing
> > possible, but it has defined behaviour by default which seems good.
> > 
> > Whatever solution we choose, I think it should be local to the null
> > driver, i.e. I don't like (2).
> 
> Neither do I, really.
> 
> Having read all of the responses, I think I'd like your approach of
> making read-zeroes the default, and people who "really know what they
> want" can turn it off and enter the danger zone if they'd like to.

I expected someone to complain about compatibility, but if nobody
objects, let's do this.

Kevin