[Qemu-commits] [qemu/qemu] eb8720: rbd: Reject -blockdev server.*.{numeric, to, ipv4, ...

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: eb87203b646c27938ecb11d7766c5c98f836241f
      
https://github.com/qemu/qemu/commit/eb87203b646c27938ecb11d7766c5c98f836241f
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M qapi-schema.json
    M qapi/block-core.json

  Log Message:
  -----------
  rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}

We use InetSocketAddress in the QAPI schema.  However, the code
doesn't use inet_connect_saddr(), but formats "host" and "port" into a
configuration string for rados_conf_set().  Thus, members "numeric",
"to", "ipv4" and "ipv6" are silently ignored.  Not nice.  Example:

    -blockdev 
rbd,node-name=nn,pool=p,image=i,server.0.host=h0,server.0.port=12345,server.0.ipv4=off

Factor a suitable InetSocketAddressBase out of InetSocketAddress, and
use that.  "numeric", "to", "ipv4" and "ipv6" are now rejected.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: f51c363c2ba97af02fedaeb7eaad68f433007382
      
https://github.com/qemu/qemu/commit/f51c363c2ba97af02fedaeb7eaad68f433007382
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Fix to cleanly reject -drive without pool or image

qemu_rbd_open() neglects to check pool and image are present.  Missing
image is caught by rbd_open(), but missing pool crashes.  Reproducer:

    $ qemu-system-x86_64 -nodefaults -drive driver=rbd,id=rbd,image=i,...
    terminate called after throwing an instance of 'std::logic_error'
      what():  basic_string::_M_construct null not valid
    Aborted (core dumped)

where ... is a working server.0.{host,port} configuration.

Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
always sets both pool and image.

Doesn't affect -blockdev, because pool and image are mandatory in the
QAPI schema.

Fix by adding the missing checks.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 730b00bbfdc15f914f47e03a703fa7647c10c4a9
      
https://github.com/qemu/qemu/commit/730b00bbfdc15f914f47e03a703fa7647c10c4a9
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Don't limit length of parameter values

We laboriously enforce that parameter values are between one and some
arbitrary limit in length.  Only RBD_MAX_IMAGE_NAME_SIZE comes from
librbd.h, and I'm not sure it applies.  Where the other limits come
from is unclear.

Drop the length checking.  The limits librbd actually imposes must be
checked by librbd anyway.

There's one minor complication: BDRVRBDState member name is a
fixed-size array.  Depends on the length limit.  Make it a pointer to
a dynamically allocated string.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 8efb339dd47b164429e9a57f36ac5c3dd810a4ce
      
https://github.com/qemu/qemu/commit/8efb339dd47b164429e9a57f36ac5c3dd810a4ce
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Clean up after the previous commit

This code in qemu_rbd_parse_filename()

    found_str = qemu_rbd_next_tok(p, '\0', &p);
    p = found_str;

has no effect.  Drop it, and simplify qemu_rbd_next_tok().

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 82f20e8547ce665e9bb23fdb55374840b846c143
      
https://github.com/qemu/qemu/commit/82f20e8547ce665e9bb23fdb55374840b846c143
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...

The way we communicate extra key-value pairs from
qemu_rbd_parse_filename() to qemu_rbd_open() exposes option parameter
"keyvalue-pairs" on the command line.  It's not wanted there.  Hack:
rename the parameter to "=keyvalue-pairs" to make it inaccessible.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: cbf036b4f3da48c2be6865eed4896acbd4149bc6
      
https://github.com/qemu/qemu/commit/cbf036b4f3da48c2be6865eed4896acbd4149bc6
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Clean up runtime_opts, fix -drive to reject filename

runtime_opts is used for three different purposes:

* qemu_rbd_open() uses it to accept options it recognizes, such as
  "pool" and "image".  Other .bdrv_open() methods do it similarly.

* qemu_rbd_open() accepts additional list-valued options
  auth-supported and server, with the help of qemu_rbd_array_opts().
  The list elements are again dictionaries.  qemu_rbd_array_opts()
  uses runtime_opts to accept their members.  Thus, runtime_opts
  contains recognized sub-sub-options "auth", "host", "port" in
  addition to recognized options.  No other block driver does that.

* qemu_rbd_create() uses it to convert the QDict produced by
  qemu_rbd_parse_filename() to QemuOpts.  No other block driver does
  that.  The keys produced by qemu_rbd_parse_filename() are "pool",
  "image", "snapshot", "conf", "user" and "keyvalue-pairs".
  qemu_rbd_open() accepts these, so no additional ones here.

This is a confusing mess.  Dates back to commit 0f9d252.  First step
to clean it up is documenting runtime_opts.desc[]:

* Reorder entries to match the QAPI schema, like we do in other block
  drivers.

* Document why the schema's "server" and "auth-supported" aren't in
  .desc[].

* Document why "keyvalue-pairs", "host", "port" and "auth" are in
  .desc[], but not the schema.

* Delete "filename", because none of the three users actually uses it.
  This fixes -drive to reject parameter filename instead of silently
  ignoring it.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 078463977a6139c9725127f2a10d9cd8b9e1b4d6
      
https://github.com/qemu/qemu/commit/078463977a6139c9725127f2a10d9cd8b9e1b4d6
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Clean up qemu_rbd_create()'s detour through QemuOpts

The conversion from QDict to QemuOpts is pointless.  Simply get the
stuff straight from the QDict.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Kevin Wolf <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 464444fcc161284ac0e743b99251debc297d5236
      
https://github.com/qemu/qemu/commit/464444fcc161284ac0e743b99251debc297d5236
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c
    M qapi/block-core.json

  Log Message:
  -----------
  rbd: Revert -blockdev and -drive parameter auth-supported

This reverts half of commit 0a55679.  We're having second thoughts on
the QAPI schema (and thus the external interface), and haven't reached
consensus, yet.  Issues include:

* The implementation uses deprecated rados_conf_set() key
  "auth_supported".  No biggie.

* The implementation makes -drive silently ignore invalid parameters
  "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
  fact I'm going to fix similar bugs around parameter server), so
  again no biggie.

* BlockdevOptionsRbd member @password-secret applies only to
  authentication method cephx.  Should it be a variant member of
  RbdAuthMethod?

* BlockdevOptionsRbd member @user could apply to both methods cephx
  and none, but I'm not sure it's actually used with none.  If it
  isn't, should it be a variant member of RbdAuthMethod?

* The client offers a *set* of authentication methods, not a list.
  Should the methods be optional members of BlockdevOptionsRbd instead
  of members of list @auth-supported?  The latter begs the question
  what multiple entries for the same method mean.  Trivial question
  now that RbdAuthMethod contains nothing but @type, but less so when
  RbdAuthMethod acquires other members, such the ones discussed above.

* How BlockdevOptionsRbd member @auth-supported interacts with
  settings from a configuration file specified with @conf is
  undocumented.  I suspect it's untested, too.

Let's avoid painting ourselves into a corner now, and revert the
feature for 2.9.

Note that users can still configure authentication methods with a
configuration file.  They probably do that anyway if they use Ceph
outside QEMU as well.

Further note that this doesn't affect use of key "auth-supported" in
-drive file=rbd:...:key=value.

qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
which is silly.  This will be cleaned up shortly.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 577d8c9a811fc697b4cc68fbbe5c509b028e0768
      
https://github.com/qemu/qemu/commit/577d8c9a811fc697b4cc68fbbe5c509b028e0768
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M qapi/block-core.json

  Log Message:
  -----------
  rbd: Revert -blockdev parameter password-secret

This reverts a part of commit 8a47e8e.  We're having second thoughts
on the QAPI schema (and thus the external interface), and haven't
reached consensus, yet.  Issues include:

* BlockdevOptionsRbd member @password-secret isn't actually a
  password, it's a key generated by Ceph.

* We're not sure where member @password-secret belongs (see the
  previous commit).

* How @password-secret interacts with settings from a configuration
  file specified with @conf is undocumented.

Let's avoid painting ourselves into a corner now, and revert the
feature for 2.9.

Note that users can still configure an authentication key with a
configuration file.  They probably do that anyway if they use Ceph
outside QEMU as well.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: 2836284db603775af557e969d5a800efb0190324
      
https://github.com/qemu/qemu/commit/2836284db603775af557e969d5a800efb0190324
  Author: Markus Armbruster <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c

  Log Message:
  -----------
  rbd: Fix bugs around -drive parameter "server"

qemu_rbd_open() takes option parameters as a flattened QDict, with
keys of the form server.%d.host, server.%d.port, where %d counts up
from zero.

qemu_rbd_array_opts() extracts these values as follows.  First, it
calls qdict_array_entries() to find the list's length.  For each list
element, it formats the list's key prefix (e.g. "server.0."), then
creates a new QDict holding the options with that key prefix, then
converts that to a QemuOpts, so it can finally get the member values
from there.

If there's one surefire way to make code using QDict more awkward,
it's creating more of them and mixing in QemuOpts for good measure.

The extraction of keys starting with server.%d into another QDict
makes us ignore parameters like server.0.neither-host-nor-port
silently.

The conversion to QemuOpts abuses runtime_opts, as described a few
commits ago.

Rewrite to simply get the values straight from the options QDict.

Fixes -drive not to crash when server.*.* are present, but
server.*.host is absent.

Fixes -drive to reject invalid server.*.*.

Permits cleaning up runtime_opts.  Do that, and fix -drive to reject
bogus parameters host and port instead of silently ignoring them.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Message-id: address@hidden
Signed-off-by: Jeff Cody <address@hidden>


  Commit: aba0fb1e2e8a6b5c76c07a1d24c587d77154491d
      
https://github.com/qemu/qemu/commit/aba0fb1e2e8a6b5c76c07a1d24c587d77154491d
  Author: Peter Maydell <address@hidden>
  Date:   2017-03-28 (Tue, 28 Mar 2017)

  Changed paths:
    M block/rbd.c
    M qapi-schema.json
    M qapi/block-core.json

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/cody/tags/block-pull-request' into 
staging

# gpg: Signature made Tue 28 Mar 2017 15:02:40 BST
# gpg:                using RSA key 0xBDBE7B27C0DE3057
# gpg: Good signature from "Jeffrey Cody <address@hidden>"
# gpg:                 aka "Jeffrey Cody <address@hidden>"
# gpg:                 aka "Jeffrey Cody <address@hidden>"
# Primary key fingerprint: 9957 4B4D 3474 90E7 9D98  D624 BDBE 7B27 C0DE 3057

* remotes/cody/tags/block-pull-request:
  rbd: Fix bugs around -drive parameter "server"
  rbd: Revert -blockdev parameter password-secret
  rbd: Revert -blockdev and -drive parameter auth-supported
  rbd: Clean up qemu_rbd_create()'s detour through QemuOpts
  rbd: Clean up runtime_opts, fix -drive to reject filename
  rbd: Don't accept -drive driver=rbd, keyvalue-pairs=...
  rbd: Clean up after the previous commit
  rbd: Don't limit length of parameter values
  rbd: Fix to cleanly reject -drive without pool or image
  rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/4d2bee82f427...aba0fb1e2e8a