diff mbox

[v6,0/2] allow blockdev-add for NFS

Message ID 50472f2d-4a71-5f6b-1037-bbefd2d8f4f4@kamp.de
State New
Headers show

Commit Message

Peter Lieven Jan. 17, 2017, 3:14 p.m. UTC
Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
> Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
>> Previously posted series patches:
>> v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
>> v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
>> v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
>> v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
>>
>> This series adds blockdev-add support for NFS block driver.
> Thanks, fixed as commented on patch 1 and applied.

Hi,

it seems this series breaks passing options via URI.

1) in nfs_parse_uri

parse_uint_full(qp->p[i].value, NULL, 0)

segfaults, as the routine wants to set *NULL = val.

Program received signal SIGSEGV, Segmentation fault.
parse_uint (s=0x555555d0ead0 "131072", value=value@entry=0x0, endptr=endptr@entry=0x7fffffffd870, base=base@entry=0) at util/cutils.c:475
475        *value = val;
(gdb) bt full
#0  parse_uint (s=0x555555d0ead0 "131072", value=value@entry=0x0, endptr=endptr@entry=0x7fffffffd870, base=base@entry=0) at util/cutils.c:475
         r = 0
         endp = 0x555555d0ead6 ""
         val = 131072
#1  0x0000555555655ff2 in parse_uint_full (s=<optimized out>, value=value@entry=0x0, base=base@entry=0) at util/cutils.c:499
         endp = 0x555555d093f0 "\004"
         r = <optimized out>
#2  0x0000555555603425 in nfs_parse_uri (filename=<optimized out>, options=0x555555d093f0, errp=0x7fffffffd980) at block/nfs.c:116
         uri = 0x555555d0e920
         qp = 0x555555d0ea30
         ret = -22
         i = 0
         __func__ = "nfs_parse_uri"
#3  0x000055555559c7bb in bdrv_fill_options (errp=0x7fffffffd968, flags=0x7fffffffd95c, filename=<optimized out>, options=<synthetischer Zeiger>) at block.c:1278
         drvname = <optimized out>
         protocol = <optimized out>
         local_err = 0x0
         parse_filename = true
         drv = 0x5555558e3140 <bdrv_nfs>


2) all parameters that have a different names in options and qdict
e.g. readahead-size vs. readahead cannot be passed via URI.

$ qemu-img convert -p nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072 iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0

qemu-img: Could not open 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072': Block protocol 'nfs' doesn't support the option 'readahead-size'

Please let me know if the below fix would be correct:

lieven@lieven-pc:~/git/qemu$ git diff


Thanks,
Peter

Comments

Kevin Wolf Jan. 18, 2017, 9:59 a.m. UTC | #1
Am 17.01.2017 um 16:14 hat Peter Lieven geschrieben:
> Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
> >Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
> >>Previously posted series patches:
> >>v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
> >>v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
> >>v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
> >>v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
> >>v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
> >>
> >>This series adds blockdev-add support for NFS block driver.
> >Thanks, fixed as commented on patch 1 and applied.
> 
> Hi,
> 
> it seems this series breaks passing options via URI.
> 
> 1) in nfs_parse_uri
> 
> parse_uint_full(qp->p[i].value, NULL, 0)
> 
> segfaults, as the routine wants to set *NULL = val.

Yes, you're right.

> 2) all parameters that have a different names in options and qdict
> e.g. readahead-size vs. readahead cannot be passed via URI.
> 
> $ qemu-img convert -p nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072 iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0
> 
> qemu-img: Could not open 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072': Block protocol 'nfs' doesn't support the option 'readahead-size'
> 
> Please let me know if the below fix would be correct:

No, this needs to be fixed the other way round: runtime_opts must use
the names as specified in the schema, and nfs_client_open() must access
them as such. Without that, blockdev-add can't work (and the command
line only with the "wrong" old option names from the URL, whereas it
should be using the same names as the QAPI schema).

Kevin
Peter Lieven Jan. 19, 2017, 2:30 p.m. UTC | #2
Am 18.01.2017 um 10:59 schrieb Kevin Wolf:
> Am 17.01.2017 um 16:14 hat Peter Lieven geschrieben:
>> Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
>>> Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
>>>> Previously posted series patches:
>>>> v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
>>>> v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
>>>> v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
>>>> v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
>>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
>>>>
>>>> This series adds blockdev-add support for NFS block driver.
>>> Thanks, fixed as commented on patch 1 and applied.
>> Hi,
>>
>> it seems this series breaks passing options via URI.
>>
>> 1) in nfs_parse_uri
>>
>> parse_uint_full(qp->p[i].value, NULL, 0)
>>
>> segfaults, as the routine wants to set *NULL = val.
> Yes, you're right.
>
>> 2) all parameters that have a different names in options and qdict
>> e.g. readahead-size vs. readahead cannot be passed via URI.
>>
>> $ qemu-img convert -p nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072 iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0
>>
>> qemu-img: Could not open 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072': Block protocol 'nfs' doesn't support the option 'readahead-size'
>>
>> Please let me know if the below fix would be correct:
> No, this needs to be fixed the other way round: runtime_opts must use
> the names as specified in the schema, and nfs_client_open() must access
> them as such. Without that, blockdev-add can't work (and the command
> line only with the "wrong" old option names from the URL, whereas it
> should be using the same names as the QAPI schema).

Shouldn't we support both for backwards compatiblity.?

Peter
Eric Blake Jan. 19, 2017, 2:59 p.m. UTC | #3
On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>> qemu-img: Could not open
>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>
>>> Please let me know if the below fix would be correct:
>> No, this needs to be fixed the other way round: runtime_opts must use
>> the names as specified in the schema, and nfs_client_open() must access
>> them as such. Without that, blockdev-add can't work (and the command
>> line only with the "wrong" old option names from the URL, whereas it
>> should be using the same names as the QAPI schema).
> 
> Shouldn't we support both for backwards compatiblity.?

blockdev-add only needs to support the modern naming.  But yes,
preserving back-compat spelling of the command-line spellings, as well
as matching blockdev-add spellings, is desirable.
Kevin Wolf Jan. 19, 2017, 3:20 p.m. UTC | #4
Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>> qemu-img: Could not open
> >>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>> Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>
> >>> Please let me know if the below fix would be correct:
> >> No, this needs to be fixed the other way round: runtime_opts must use
> >> the names as specified in the schema, and nfs_client_open() must access
> >> them as such. Without that, blockdev-add can't work (and the command
> >> line only with the "wrong" old option names from the URL, whereas it
> >> should be using the same names as the QAPI schema).
> > 
> > Shouldn't we support both for backwards compatiblity.?
> 
> blockdev-add only needs to support the modern naming.  But yes,
> preserving back-compat spelling of the command-line spellings, as well
> as matching blockdev-add spellings, is desirable.

We only just added the individual command line options, previously it
only supported the URL.

It's true that we have the messed up version of the options in 2.8, so
strictly speaking we would break compatibility with a release, but it's
only one release, it's only the nfs driver, and the documentation of the
options is the schema, which had the right option names even in 2.8
(they just didn't work).

So I wouldn't feel bad about removing the wrong names in this specific
case.

Kevin
Peter Lieven Jan. 19, 2017, 3:34 p.m. UTC | #5
Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>> qemu-img: Could not open
>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>
>>>>> Please let me know if the below fix would be correct:
>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>> the names as specified in the schema, and nfs_client_open() must access
>>>> them as such. Without that, blockdev-add can't work (and the command
>>>> line only with the "wrong" old option names from the URL, whereas it
>>>> should be using the same names as the QAPI schema).
>>> Shouldn't we support both for backwards compatiblity.?
>> blockdev-add only needs to support the modern naming.  But yes,
>> preserving back-compat spelling of the command-line spellings, as well
>> as matching blockdev-add spellings, is desirable.
> We only just added the individual command line options, previously it
> only supported the URL.
>
> It's true that we have the messed up version of the options in 2.8, so
> strictly speaking we would break compatibility with a release, but it's
> only one release, it's only the nfs driver, and the documentation of the
> options is the schema, which had the right option names even in 2.8
> (they just didn't work).
>
> So I wouldn't feel bad about removing the wrong names in this specific
> case.

So want exactly do you want to do? Fix the names in the QAPI schema
to use the old naming?

Peter
Kevin Wolf Jan. 19, 2017, 3:42 p.m. UTC | #6
Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> >Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> >>On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>>>>qemu-img: Could not open
> >>>>>'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>>>>Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>>>
> >>>>>Please let me know if the below fix would be correct:
> >>>>No, this needs to be fixed the other way round: runtime_opts must use
> >>>>the names as specified in the schema, and nfs_client_open() must access
> >>>>them as such. Without that, blockdev-add can't work (and the command
> >>>>line only with the "wrong" old option names from the URL, whereas it
> >>>>should be using the same names as the QAPI schema).
> >>>Shouldn't we support both for backwards compatiblity.?
> >>blockdev-add only needs to support the modern naming.  But yes,
> >>preserving back-compat spelling of the command-line spellings, as well
> >>as matching blockdev-add spellings, is desirable.
> >We only just added the individual command line options, previously it
> >only supported the URL.
> >
> >It's true that we have the messed up version of the options in 2.8, so
> >strictly speaking we would break compatibility with a release, but it's
> >only one release, it's only the nfs driver, and the documentation of the
> >options is the schema, which had the right option names even in 2.8
> >(they just didn't work).
> >
> >So I wouldn't feel bad about removing the wrong names in this specific
> >case.
> 
> So want exactly do you want to do? Fix the names in the QAPI schema
> to use the old naming?

No, fix the command line to use the names in the QAPI schema.

The option names from the URL were never supposed to be supported on the
command line.

Kevin
Peter Lieven Jan. 19, 2017, 3:44 p.m. UTC | #7
Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>> qemu-img: Could not open
>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>
>>>>>>> Please let me know if the below fix would be correct:
>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>> should be using the same names as the QAPI schema).
>>>>> Shouldn't we support both for backwards compatiblity.?
>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>> preserving back-compat spelling of the command-line spellings, as well
>>>> as matching blockdev-add spellings, is desirable.
>>> We only just added the individual command line options, previously it
>>> only supported the URL.
>>>
>>> It's true that we have the messed up version of the options in 2.8, so
>>> strictly speaking we would break compatibility with a release, but it's
>>> only one release, it's only the nfs driver, and the documentation of the
>>> options is the schema, which had the right option names even in 2.8
>>> (they just didn't work).
>>>
>>> So I wouldn't feel bad about removing the wrong names in this specific
>>> case.
>> So want exactly do you want to do? Fix the names in the QAPI schema
>> to use the old naming?
> No, fix the command line to use the names in the QAPI schema.
>
> The option names from the URL were never supposed to be supported on the
> command line.

Okay, so no backwards compatiblity? I actually used the options on the command line...

Peter
Kevin Wolf Jan. 19, 2017, 3:55 p.m. UTC | #8
Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
> Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> >Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
> >>Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> >>>Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> >>>>On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>>>>>>qemu-img: Could not open
> >>>>>>>'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>>>>>>Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>>>>>
> >>>>>>>Please let me know if the below fix would be correct:
> >>>>>>No, this needs to be fixed the other way round: runtime_opts must use
> >>>>>>the names as specified in the schema, and nfs_client_open() must access
> >>>>>>them as such. Without that, blockdev-add can't work (and the command
> >>>>>>line only with the "wrong" old option names from the URL, whereas it
> >>>>>>should be using the same names as the QAPI schema).
> >>>>>Shouldn't we support both for backwards compatiblity.?
> >>>>blockdev-add only needs to support the modern naming.  But yes,
> >>>>preserving back-compat spelling of the command-line spellings, as well
> >>>>as matching blockdev-add spellings, is desirable.
> >>>We only just added the individual command line options, previously it
> >>>only supported the URL.
> >>>
> >>>It's true that we have the messed up version of the options in 2.8, so
> >>>strictly speaking we would break compatibility with a release, but it's
> >>>only one release, it's only the nfs driver, and the documentation of the
> >>>options is the schema, which had the right option names even in 2.8
> >>>(they just didn't work).
> >>>
> >>>So I wouldn't feel bad about removing the wrong names in this specific
> >>>case.
> >>So want exactly do you want to do? Fix the names in the QAPI schema
> >>to use the old naming?
> >No, fix the command line to use the names in the QAPI schema.
> >
> >The option names from the URL were never supposed to be supported on the
> >command line.
> 
> Okay, so no backwards compatiblity? I actually used the options on the command line...

Well, do you _need_ compatibility?

It can certainly be done, but as the (wrong) options on the command line
have only existed since November and were never documented, I wouldn't
bother unless there's a good reason.

Kevin
Peter Lieven Jan. 19, 2017, 3:58 p.m. UTC | #9
Am 19.01.2017 um 16:55 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>>>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>>>> qemu-img: Could not open
>>>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>>>
>>>>>>>>> Please let me know if the below fix would be correct:
>>>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>>>> should be using the same names as the QAPI schema).
>>>>>>> Shouldn't we support both for backwards compatiblity.?
>>>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>>>> preserving back-compat spelling of the command-line spellings, as well
>>>>>> as matching blockdev-add spellings, is desirable.
>>>>> We only just added the individual command line options, previously it
>>>>> only supported the URL.
>>>>>
>>>>> It's true that we have the messed up version of the options in 2.8, so
>>>>> strictly speaking we would break compatibility with a release, but it's
>>>>> only one release, it's only the nfs driver, and the documentation of the
>>>>> options is the schema, which had the right option names even in 2.8
>>>>> (they just didn't work).
>>>>>
>>>>> So I wouldn't feel bad about removing the wrong names in this specific
>>>>> case.
>>>> So want exactly do you want to do? Fix the names in the QAPI schema
>>>> to use the old naming?
>>> No, fix the command line to use the names in the QAPI schema.
>>>
>>> The option names from the URL were never supposed to be supported on the
>>> command line.
>> Okay, so no backwards compatiblity? I actually used the options on the command line...
> Well, do you _need_ compatibility?
>
> It can certainly be done, but as the (wrong) options on the command line
> have only existed since November and were never documented, I wouldn't
> bother unless there's a good reason.

Every Qemu before 2.8.0 was working with sth like:

qemu -cdrom nfs://10.0.0.1/expory/my.iso?readahead=131072

Peter
Kevin Wolf Jan. 19, 2017, 5:08 p.m. UTC | #10
Am 19.01.2017 um 16:58 hat Peter Lieven geschrieben:
> Am 19.01.2017 um 16:55 schrieb Kevin Wolf:
> >Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
> >>Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> >>>Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
> >>>>Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> >>>>>Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> >>>>>>On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>>>>>>>>qemu-img: Could not open
> >>>>>>>>>'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>>>>>>>>Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>>>>>>>
> >>>>>>>>>Please let me know if the below fix would be correct:
> >>>>>>>>No, this needs to be fixed the other way round: runtime_opts must use
> >>>>>>>>the names as specified in the schema, and nfs_client_open() must access
> >>>>>>>>them as such. Without that, blockdev-add can't work (and the command
> >>>>>>>>line only with the "wrong" old option names from the URL, whereas it
> >>>>>>>>should be using the same names as the QAPI schema).
> >>>>>>>Shouldn't we support both for backwards compatiblity.?
> >>>>>>blockdev-add only needs to support the modern naming.  But yes,
> >>>>>>preserving back-compat spelling of the command-line spellings, as well
> >>>>>>as matching blockdev-add spellings, is desirable.
> >>>>>We only just added the individual command line options, previously it
> >>>>>only supported the URL.
> >>>>>
> >>>>>It's true that we have the messed up version of the options in 2.8, so
> >>>>>strictly speaking we would break compatibility with a release, but it's
> >>>>>only one release, it's only the nfs driver, and the documentation of the
> >>>>>options is the schema, which had the right option names even in 2.8
> >>>>>(they just didn't work).
> >>>>>
> >>>>>So I wouldn't feel bad about removing the wrong names in this specific
> >>>>>case.
> >>>>So want exactly do you want to do? Fix the names in the QAPI schema
> >>>>to use the old naming?
> >>>No, fix the command line to use the names in the QAPI schema.
> >>>
> >>>The option names from the URL were never supposed to be supported on the
> >>>command line.
> >>Okay, so no backwards compatiblity? I actually used the options on the command line...
> >Well, do you _need_ compatibility?
> >
> >It can certainly be done, but as the (wrong) options on the command line
> >have only existed since November and were never documented, I wouldn't
> >bother unless there's a good reason.
> 
> Every Qemu before 2.8.0 was working with sth like:
> 
> qemu -cdrom nfs://10.0.0.1/expory/my.iso?readahead=131072

That will keep working. We're not changing the URL parsing, just the
runtime_opts and its accesses in nfs_client_open(). The translation in
nfs_parse_uri() stays intact with the fixes.

What will stop working (and only worked in 2.8.0) is this:

    qemu -drive media=cdrom,driver=nfs,server.host=10.0.0.1,path=export/my.iso,readahead=131072

Also, I think the fixes should be Cc: qemu-stable, so that 2.8.1 will
work correctly again.

Kevin
Peter Lieven Jan. 19, 2017, 5:13 p.m. UTC | #11
Am 19.01.2017 um 18:08 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:58 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:55 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
>>>> Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
>>>>> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>>>>>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>>>>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>>>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>>>>>> qemu-img: Could not open
>>>>>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>>>>>
>>>>>>>>>>> Please let me know if the below fix would be correct:
>>>>>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>>>>>> should be using the same names as the QAPI schema).
>>>>>>>>> Shouldn't we support both for backwards compatiblity.?
>>>>>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>>>>>> preserving back-compat spelling of the command-line spellings, as well
>>>>>>>> as matching blockdev-add spellings, is desirable.
>>>>>>> We only just added the individual command line options, previously it
>>>>>>> only supported the URL.
>>>>>>>
>>>>>>> It's true that we have the messed up version of the options in 2.8, so
>>>>>>> strictly speaking we would break compatibility with a release, but it's
>>>>>>> only one release, it's only the nfs driver, and the documentation of the
>>>>>>> options is the schema, which had the right option names even in 2.8
>>>>>>> (they just didn't work).
>>>>>>>
>>>>>>> So I wouldn't feel bad about removing the wrong names in this specific
>>>>>>> case.
>>>>>> So want exactly do you want to do? Fix the names in the QAPI schema
>>>>>> to use the old naming?
>>>>> No, fix the command line to use the names in the QAPI schema.
>>>>>
>>>>> The option names from the URL were never supposed to be supported on the
>>>>> command line.
>>>> Okay, so no backwards compatiblity? I actually used the options on the command line...
>>> Well, do you _need_ compatibility?
>>>
>>> It can certainly be done, but as the (wrong) options on the command line
>>> have only existed since November and were never documented, I wouldn't
>>> bother unless there's a good reason.
>> Every Qemu before 2.8.0 was working with sth like:
>>
>> qemu -cdrom nfs://10.0.0.1/expory/my.iso?readahead=131072
> That will keep working. We're not changing the URL parsing, just the
> runtime_opts and its accesses in nfs_client_open(). The translation in
> nfs_parse_uri() stays intact with the fixes.
>
> What will stop working (and only worked in 2.8.0) is this:
>
>      qemu -drive media=cdrom,driver=nfs,server.host=10.0.0.1,path=export/my.iso,readahead=131072
>
> Also, I think the fixes should be Cc: qemu-stable, so that 2.8.1 will
> work correctly again.

Okay, I hope I got it. Will send a patch tomorrow.

Peter
diff mbox

Patch

diff --git a/block/nfs.c b/block/nfs.c
index a564340..0ff8268 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -108,30 +108,31 @@  static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
      qdict_put(options, "path", qstring_from_str(uri->path));

      for (i = 0; i < qp->n; i++) {
+        unsigned long long val;
          if (!qp->p[i].value) {
              error_setg(errp, "Value for NFS parameter expected: %s",
                         qp->p[i].name);
              goto out;
          }
-        if (parse_uint_full(qp->p[i].value, NULL, 0)) {
+        if (parse_uint_full(qp->p[i].value, &val, 0)) {
              error_setg(errp, "Illegal value for NFS parameter: %s",
                         qp->p[i].name);
              goto out;
          }
          if (!strcmp(qp->p[i].name, "uid")) {
-            qdict_put(options, "user",
+            qdict_put(options, "uid",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "gid")) {
-            qdict_put(options, "group",
+            qdict_put(options, "gid",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-            qdict_put(options, "tcp-syn-count",
+            qdict_put(options, "tcp-syncnt",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "readahead")) {
-            qdict_put(options, "readahead-size",
+            qdict_put(options, "readahead",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "pagecache")) {
-            qdict_put(options, "page-cache-size",
+            qdict_put(options, "pagecache",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "debug")) {
              qdict_put(options, "debug",