All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Peter Lieven <pl@kamp.de>
Cc: Eric Blake <eblake@redhat.com>,
	Ashijeet Acharya <ashijeetacharya@gmail.com>,
	jcody@redhat.com, mreitz@redhat.com, armbru@redhat.com,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS
Date: Thu, 19 Jan 2017 18:08:30 +0100	[thread overview]
Message-ID: <20170119170830.GD5443@noname.redhat.com> (raw)
In-Reply-To: <97063398-5dab-a17f-253d-c0be1c34cfa8@kamp.de>

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

  reply	other threads:[~2017-01-19 17:08 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-31 15:05 [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS Ashijeet Acharya
2016-10-31 15:05 ` [Qemu-devel] [PATCH v6 1/2] block/nfs: Introduce runtime_opts in NFS Ashijeet Acharya
2016-10-31 15:48   ` Kevin Wolf
2016-10-31 15:05 ` [Qemu-devel] [PATCH v6 2/2] qapi: allow blockdev-add for NFS Ashijeet Acharya
2016-10-31 17:20 ` [Qemu-devel] [PATCH v6 0/2] " Kevin Wolf
2017-01-17 15:14   ` Peter Lieven
2017-01-18  9:59     ` Kevin Wolf
2017-01-19 14:30       ` Peter Lieven
2017-01-19 14:59         ` Eric Blake
2017-01-19 15:20           ` Kevin Wolf
2017-01-19 15:34             ` Peter Lieven
2017-01-19 15:42               ` Kevin Wolf
2017-01-19 15:44                 ` Peter Lieven
2017-01-19 15:55                   ` Kevin Wolf
2017-01-19 15:58                     ` Peter Lieven
2017-01-19 17:08                       ` Kevin Wolf [this message]
2017-01-19 17:13                         ` Peter Lieven
2017-01-19 15:47                 ` Peter Lieven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170119170830.GD5443@noname.redhat.com \
    --to=kwolf@redhat.com \
    --cc=armbru@redhat.com \
    --cc=ashijeetacharya@gmail.com \
    --cc=eblake@redhat.com \
    --cc=jcody@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pl@kamp.de \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.