Libguestfs - Mar 2018 - Re: [PATCH] tests: regressions: make test-big-heap use a temporary empty file

[adding qemu lists]

On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
>> Newer versions of qemu use file locking for the images by default, and
>> apparently that does not work with /dev/null.  Since this test just
>> calls qemu-img to get the format of an empty image, create a temporary
>> one instead.
> 
> ACK, but feels like this is a bug in qemu-img to me.
You're right that file locking on a character device like /dev/null is 
not going to work as expected, but is it a case where fcntl() actually 
fails, or is it worse where the fcntl() claiming the locks "succeeds" 
but doesn't do what we want?  That is, what were the actual error 
messages you ran into?

Is it something where qemu should instead be checking fstat() results 
and only doing locking on regular files and block devices?  At any rate, 
I agree that qemu itself should behave nicer on attempts to use 
/dev/null as an image.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Wednesday, 21 March 2018 14:45:38 CET Eric Blake
wrote:
> [adding qemu lists]
> 
> On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> > On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
> >> Newer versions of qemu use file locking for the images by default,
and
> >> apparently that does not work with /dev/null.  Since this test
just
> >> calls qemu-img to get the format of an empty image, create a
temporary
> >> one instead.
> > 
> > ACK, but feels like this is a bug in qemu-img to me.
> 
> You're right that file locking on a character device like /dev/null is 
> not going to work as expected, but is it a case where fcntl() actually 
> fails, or is it worse where the fcntl() claiming the locks
"succeeds"
> but doesn't do what we want?  That is, what were the actual error 
> messages you ran into?
$ qemu-img --version
qemu-img version 2.10.1(qemu-2.10.1-2.fc27)
Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
$ qemu-img info /dev/null 
qemu-img: Could not open '/dev/null': Failed to get "consistent
read" lock
Is another process using the image?

-- 
Pino Toscano

Am 21.03.2018 um 14:48 hat Pino Toscano geschrieben:
> On Wednesday, 21 March 2018 14:45:38 CET Eric Blake wrote:
> > [adding qemu lists]
> > 
> > On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> > > On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
> > >> Newer versions of qemu use file locking for the images by
default, and
> > >> apparently that does not work with /dev/null.  Since this
test just
> > >> calls qemu-img to get the format of an empty image, create a
temporary
> > >> one instead.
> > > 
> > > ACK, but feels like this is a bug in qemu-img to me.
> > 
> > You're right that file locking on a character device like
/dev/null is
> > not going to work as expected, but is it a case where fcntl() actually
> > fails, or is it worse where the fcntl() claiming the locks
"succeeds"
> > but doesn't do what we want?  That is, what were the actual error 
> > messages you ran into?
> 
> $ qemu-img --version
> qemu-img version 2.10.1(qemu-2.10.1-2.fc27)
> Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
> $ qemu-img info /dev/null 
> qemu-img: Could not open '/dev/null': Failed to get
"consistent read" lock
> Is another process using the image?
Not sure where the difference is, but I can't reproduce this on
upstream, neither git master nor the v2.10.1 tag:

$ ./qemu-img --version
qemu-img version 2.10.1 (v2.10.1-dirty)
Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
$ ./qemu-img info /dev/null
image: /dev/null
file format: raw
virtual size: 0 (0 bytes)
disk size: 0

Also with strace:

open("/dev/null", O_RDONLY|O_CLOEXEC)   = 10
fcntl(10, F_OFD_SETLK, {l_type=F_RDLCK, l_whence=SEEK_SET, l_start=100,
l_len=1}) = 0
...

So my kernel (4.15.9-200.fc26.x86_64) seems to have no problems with
locking /dev/null.

Kevin