Re: [Qemu-block] [PATCH v2 06/21] block: Exclude nested options only for children in append_open_options()

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> Some drivers have nested options (e.g. blkdebug rule arrays), which
> don't belong to a child node and shouldn't be removed. Don't remove all
> options with "." in their name, but check for the complete prefixes of
> actually existing child nodes.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 19 +++
>  include/block/block_int.h |  1 +
>  2 files changed, 16 insertions(+), 4 deletions(-)

Thanks, now I don't need to fix it myself. :-)

(I would have had to do that for an in-work series of mine)

> diff --git a/block.c b/block.c
> index 23d9e10..02125e2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const 
> char **pfilename,
>  
>  static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>  BlockDriverState *child_bs,
> +const char *child_name,
>  const BdrvChildRole *child_role)
>  {
>  BdrvChild *child = g_new(BdrvChild, 1);
>  *child = (BdrvChild) {
>  .bs = child_bs,
> +.name   = child_name,
>  .role   = child_role,
>  };
>  
> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>  bs->backing = NULL;
>  goto out;
>  }
> -bs->backing = bdrv_attach_child(bs, backing_hd, _backing);
> +bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> _backing);
>  bs->open_flags &= ~BDRV_O_NO_BACKING;
>  pstrcpy(bs->backing_file, sizeof(bs->backing_file), 
> backing_hd->filename);
>  pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename,
>  goto done;
>  }
>  
> -c = bdrv_attach_child(parent, bs, child_role);
> +c = bdrv_attach_child(parent, bs, bdref_key, child_role);
>  
>  done:
>  qdict_del(options, bdref_key);
> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, 
> BlockDriverState *bs)
>  {
>  const QDictEntry *entry;
>  QemuOptDesc *desc;
> +BdrvChild *child;
>  bool found_any = false;
> +const char *p;
>  
>  for (entry = qdict_first(bs->options); entry;
>   entry = qdict_next(bs->options, entry))
>  {
> -/* Only take options for this level */
> -if (strchr(qdict_entry_key(entry), '.')) {
> +/* Exclude options for children */
> +QLIST_FOREACH(child, >children, next) {
> +if (strstart(qdict_entry_key(entry), child->name, )
> +&& (!*p || *p == '.'))
> +{
> +break;
> +}
> +}
> +if (child) {
>  continue;
>  }
>  

A good general solution, but I think bdrv_refresh_filename() may be bad
enough to break with general solutions. ;-)

bdrv_refresh_filename() only considers "file" and "backing" (actually,
it only supports "file" for now, I'm working on "backing", though). The
only drivers with other children are quorum, blkdebug, blkverify and
VMDK. The former three have their own implementation of
bdrv_refresh_filename(), so they don't use append_open_options() at all.
The latter, however, (VMDK) does not.

This change to append_open_options results in the extent.%d options
simply being omitted altogether because bdrv_refresh_filename() does not
fetch them. Before, they were included in the VMDK BDS's options, which
is not ideal but works more or less.

In order to "fix" this, I see three ways right now:
1. Just care about "file" and "backing". bdrv_refresh_filename()
   doesn't support anything else, so that will be fine.
2. Implement bdrv_refresh_filename() specifically for VMDK so
   append_open_options() will never have to handle anything but "file"
   and "backing".
3. Fix bdrv_refresh_filename() so that it handles all children and not
   just "file" and "backing".

Since we are shooting for 2.6 anyway (I assume ;-)), I think we should
go for option 3. This means that this patch is fine, and I'll see to
fixing bdrv_refresh_filename() (because I'm working on that anyway).

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 14/21] blockdev: Set 'format' indicates non-empty drive

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> Creating an empty drive while specifying 'format' doesn't make sense.
> The specified format driver would simply be ignored.
> 
> Make a set 'format' option an indication that a non-empty drive should
> be created. This makes 'format' consistent with 'driver' and allows
> using it with a block driver that doesn't need any other options (like
> null-co/null-aio).
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockdev.c| 5 +
>  tests/qemu-iotests/iotests.py | 2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)

Besides the failing make check, the patch looks good.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 18/21] blkdebug: Enable reopen

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> Just reopening the children (as block.c does now) is enough.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/blkdebug.c | 7 +++
>  1 file changed, 7 insertions(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 17/21] block: Move cache options into options QDict

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> This adds the cache mode options to the QDict, so that they can be
> specified for child nodes (e.g. backing.cache.direct=off).
> 
> The cache modes are not removed from the flags at this point; instead,
> options and flags are kept in sync. If the user specifies both flags and
> options, the options take precedence.
> 
> Child node inherit cache modes as options now, they don't use flags any
> more.
> 
> Note that this forbids specifying the cache mode for empty drives. It
> didn't make sense anyway to specify it there, because it didn't have any
> effect. blockdev_init() considers the cache options now bdrv_open()
> options and therefore doesn't create an empty drive any more but calls
> into bdrv_open(). This in turn will fail with no driver and filename
> specified.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c| 100 
> ++---
>  blockdev.c |  52 ++--
>  2 files changed, 111 insertions(+), 41 deletions(-)
> 
> diff --git a/block.c b/block.c
> index eff0c19..397014a 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -1747,12 +1816,22 @@ static BlockReopenQueue 
> *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>  /*
>   * Precedence of options:
>   * 1. Explicitly passed in options (highest)
> - * 2. TODO Set in flags (only for top level)
> + * 2. Set in flags (only for top level)
>   * 3. Retained from explicitly set options of bs
>   * 4. Inherited from parent node
>   * 5. Retained from effective options of bs
>   */
>  
> +if (!parent_options) {
> +/*
> + * Any setting represented by flags is always updated. If the
> + * corresponding QDict option is set, it takes precedence. Otherwise
> + * the flag is translated into a QDict option. The old setting of bs 
> is
> + * not considered.
> + */
> +update_options_from_flags(options, flags);
> +}
> +
>  /* Old explicitly set values (don't overwrite by inherited value) */
>  old_options = qdict_clone_shallow(bs->explicit_options);
>  bdrv_join_options(bs, options, old_options);
> @@ -1923,6 +2002,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>  goto error;
>  }
>  
> +update_flags_from_options(_state->flags, opts);
> +
> +/* If a guest device is attached, it owns WCE */
> +if (reopen_state->bs->blk && 
> blk_get_attached_dev(reopen_state->bs->blk)) {
> +bool old_wce = bdrv_enable_write_cache(reopen_state->bs);
> +bool new_wce = (reopen_state->flags & BDRV_O_CACHE_WB);
> +if (old_wce != new_wce) {
> +error_setg(errp, "Cannot change cache.writeback: Device 
> attached");
> +ret = -EINVAL;
> +goto error;
> +}
> +}
> +

Time to get back to my question regarding bdrv_set_enable_write_cache():

1. bdrv_set_enable_write_cache() sets/unsets BDRV_O_CACHE_WB in
   bs->open_flags so that it is preserved through a reopen.
2. On reopen, bdrv_reopen_queue_child() calls
   update_options_from_flags() unless @parent_options is set. If that
   function is called, it will set the appropriate caching options in
   @options as dictated by bdrv_set_enable_write_cache().
3. If @parent_options was NULL, the update_flags_from_options() call
   here in bdrv_reopen_prepare() will set BDRV_O_CACHE_WB just as
   dictated by bdrv_set_enable_write_cache() (unless overwritten).
   That is what we want.
4. If @parent_options was not NULL, the caching options in
   bs->open_flags are completely overwritten, discarding everything that
   had been set by bdrv_set_enable_write_cache(). That's not so good.

@parent_options is tested so that update_options_from_flags() is called
only for root BDSs. It is possible to call bdrv_set_enable_write_cache()
on non-root BDSs (e.g. commit_active_start() does it on the commit base
via mirror_start_job()), so I do think overriding the setting made by
bdrv_set_enable_write_cache() on reopen for any non-root BDSs is not
correct.

Maybe bdrv_set_enable_write_cache() should set cache.writeback in
bs->explicit_options to on or off instead of using bs->open_flags. But
in that case, we can no longer use bdrv_set_enable_write_cache() in
bdrv_open_common(), because the cache setting there may be implicit.

Max

>  /* node-name and driver must be unchanged. Put them back into the QDict, 
> so
>   * that they are checked at the end of this function. */
>  value = qemu_opt_get(opts, "node-name");



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 13/21] block: Introduce bs->explicit_options

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> bs->options doesn't only contain options that the user explicitly
> requested, but also option that were derived from flags, the filename or
> inherited from the parent node.
> 
> For reopen, it is important to know the difference because reopening the
> parent can change inherited values in child nodes, but it shouldn't
> change any options that were explicitly specified for the child.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 24 ++--
>  include/block/block.h |  1 +
>  include/block/block_int.h |  1 +
>  3 files changed, 24 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 20/21] iotests: add incremental backup failure recovery test

[Kevin Wolf]

Am 18.04.2015 um 01:50 hat John Snow geschrieben:
> Test the failure case for incremental backups.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Max Reitz 
> ---
>  tests/qemu-iotests/124 | 57 
> ++
>  tests/qemu-iotests/124.out |  4 ++--
>  2 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
> index 5c3b434..95f6de5 100644
> --- a/tests/qemu-iotests/124
> +++ b/tests/qemu-iotests/124
> @@ -240,6 +240,63 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>  self.check_backups()
>  
>  
> +def test_incremental_failure(self):
> +'''Test: Verify backups made after a failure are correct.
> +
> +Simulate a failure during an incremental backup block job,
> +emulate additional writes, then create another incremental backup
> +afterwards and verify that the backup created is correct.
> +'''
> +
> +# Create a blkdebug interface to this img as 'drive1',
> +# but don't actually create a new image.
> +drive1 = self.add_node('drive1', self.drives[0]['fmt'],
> +   path=self.drives[0]['file'],
> +   backup=self.drives[0]['backup'])
> +result = self.vm.qmp('blockdev-add', options={
> +'id': drive1['id'],
> +'driver': drive1['fmt'],
> +'file': {
> +'driver': 'blkdebug',
> +'image': {
> +'driver': 'file',
> +'filename': drive1['file']
> +},
> +'set-state': [{
> +'event': 'flush_to_disk',
> +'state': 1,
> +'new_state': 2
> +}],
> +'inject-error': [{
> +'event': 'read_aio',
> +'errno': 5,
> +'state': 2,
> +'immediately': False,
> +'once': True
> +}],
> +}
> +})
> +self.assert_qmp(result, 'return', {})

John, how naughty of you!

It's interesting how many tests break now that I tried to add some
advisory qcow2 locking so that people don't constantly break their
images with 'qemu-img snapshot' while the VM is running.

I think this one is a bug in the test case. I'm not completely sure how
to fix it, though. Can we move the blkdebug layer to the top level? I
think reusing the same qcow2 BDS (using a node name reference) would be
okay. We just need to avoid opening the qcow2 layer twice for the same
image.

Kevin

Re: [Qemu-block] [PATCH v2 06/21] block: Exclude nested options only for children in append_open_options()

[Max Reitz]

On 27.11.2015 18:58, Max Reitz wrote:
> On 23.11.2015 16:59, Kevin Wolf wrote:
>> Some drivers have nested options (e.g. blkdebug rule arrays), which
>> don't belong to a child node and shouldn't be removed. Don't remove all
>> options with "." in their name, but check for the complete prefixes of
>> actually existing child nodes.
>>
>> Signed-off-by: Kevin Wolf 
>> ---
>>  block.c   | 19 +++
>>  include/block/block_int.h |  1 +
>>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> Thanks, now I don't need to fix it myself. :-)
> 
> (I would have had to do that for an in-work series of mine)
> 
>> diff --git a/block.c b/block.c
>> index 23d9e10..02125e2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const 
>> char **pfilename,
>>  
>>  static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>>  BlockDriverState *child_bs,
>> +const char *child_name,
>>  const BdrvChildRole *child_role)
>>  {
>>  BdrvChild *child = g_new(BdrvChild, 1);
>>  *child = (BdrvChild) {
>>  .bs = child_bs,
>> +.name   = child_name,
>>  .role   = child_role,
>>  };
>>  
>> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
>> BlockDriverState *backing_hd)
>>  bs->backing = NULL;
>>  goto out;
>>  }
>> -bs->backing = bdrv_attach_child(bs, backing_hd, _backing);
>> +bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
>> _backing);
>>  bs->open_flags &= ~BDRV_O_NO_BACKING;
>>  pstrcpy(bs->backing_file, sizeof(bs->backing_file), 
>> backing_hd->filename);
>>  pstrcpy(bs->backing_format, sizeof(bs->backing_format),
>> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename,
>>  goto done;
>>  }
>>  
>> -c = bdrv_attach_child(parent, bs, child_role);
>> +c = bdrv_attach_child(parent, bs, bdref_key, child_role);
>>  
>>  done:
>>  qdict_del(options, bdref_key);
>> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, 
>> BlockDriverState *bs)
>>  {
>>  const QDictEntry *entry;
>>  QemuOptDesc *desc;
>> +BdrvChild *child;
>>  bool found_any = false;
>> +const char *p;
>>  
>>  for (entry = qdict_first(bs->options); entry;
>>   entry = qdict_next(bs->options, entry))
>>  {
>> -/* Only take options for this level */
>> -if (strchr(qdict_entry_key(entry), '.')) {
>> +/* Exclude options for children */
>> +QLIST_FOREACH(child, >children, next) {
>> +if (strstart(qdict_entry_key(entry), child->name, )
>> +&& (!*p || *p == '.'))
>> +{
>> +break;
>> +}
>> +}
>> +if (child) {
>>  continue;
>>  }
>>  
> 
> A good general solution, but I think bdrv_refresh_filename() may be bad
> enough to break with general solutions. ;-)
> 
> bdrv_refresh_filename() only considers "file" and "backing" (actually,
> it only supports "file" for now, I'm working on "backing", though). The
> only drivers with other children are quorum, blkdebug, blkverify and
> VMDK. The former three have their own implementation of
> bdrv_refresh_filename(), so they don't use append_open_options() at all.
> The latter, however, (VMDK) does not.
> 
> This change to append_open_options results in the extent.%d options
> simply being omitted altogether because bdrv_refresh_filename() does not
> fetch them. Before, they were included in the VMDK BDS's options, which
> is not ideal but works more or less.
> 
> In order to "fix" this, I see three ways right now:
> 1. Just care about "file" and "backing". bdrv_refresh_filename()
>doesn't support anything else, so that will be fine.
> 2. Implement bdrv_refresh_filename() specifically for VMDK so
>append_open_options() will never have to handle anything but "file"
>and "backing".
> 3. Fix bdrv_refresh_filename() so that it handles all children and not
>just "file" and "backing".
> 
> Since we are shooting for 2.6 anyway (I assume ;-)), I think we should
> go for option 3. This means that this patch is fine, and I'll see to
> fixing bdrv_refresh_filename() (because I'm working on that anyway).
> 
> Reviewed-by: Max Reitz 

Oops, focused so much on this that I missed the issue Wen Congyang
found. So this R-b is assuming that is fixed *cough*.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 12/21] block: Split out parse_json_protocol()

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> The next patch distinguishes options that were explicitly set and
> options that were derived. bdrv_fill_option() added options of both
> types: Options given by json: syntax should be counted as explicit, but
> the rest is derived.
> 
> In preparation for the distinction, move json: parse to a separate
> function.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 50 --
>  1 file changed, 32 insertions(+), 18 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 03/21] mirror: Error out when a BDS would get two BBs

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> bdrv_replace_in_backing_chain() asserts that not both old and new
> BlockDdriverState have a BlockBackend attached to them because both
> would have to end up pointing to the new BDS and we don't support more
> than one BB per BDS yet.
> 
> Before we can safely allow references to existing nodes as backing
> files, we need to make sure that even if a backing file has a BB on it,
> this doesn't crash qemu.
> 
> There are probably also some cases with the 'replaces' option set where
> drive-mirror could fail this assertion today. They are fixed with this
> error check as well.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 52c9abf..0620068 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -18,6 +18,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/error-report.h"
>  
>  #define SLICE_TIME1ULL /* ns */
>  #define MAX_IN_FLIGHT 16
> @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  if (s->to_replace) {
>  to_replace = s->to_replace;
>  }
> +
> +/* This was checked in mirror_start_job(), but meanwhile one of the
> + * nodes could have been newly attached to a BlockBackend. */
> +if (to_replace->blk && s->target->blk) {
> +error_report("block job: Can't create node with two 
> BlockBackends");
> +data->ret = -EINVAL;
> +goto out;
> +}
> +
>  if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
>  bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
>  }
>  bdrv_replace_in_backing_chain(to_replace, s->target);
>  }
> +
> +out:
>  if (s->to_replace) {
>  bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
>  error_free(s->replace_blocker);
> @@ -701,6 +713,7 @@ static void mirror_start_job(BlockDriverState *bs, 
> BlockDriverState *target,
>   bool is_none_mode, BlockDriverState *base)
>  {
>  MirrorBlockJob *s;
> +BlockDriverState *replaced_bs;
>  
>  if (granularity == 0) {
>  granularity = bdrv_get_default_bitmap_granularity(target);
> @@ -724,6 +737,21 @@ static void mirror_start_job(BlockDriverState *bs, 
> BlockDriverState *target,
>  buf_size = DEFAULT_MIRROR_BUF_SIZE;
>  }
>  
> +/* We can't support this case as long as the block layer can't handle
> + * multple BlockBackends per BlockDriverState. */

*multiple

With that fixed:

Reviewed-by: Max Reitz 

> +if (replaces) {
> +replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
> +if (replaced_bs == NULL) {
> +return;
> +}
> +} else {
> +replaced_bs = bs;
> +}
> +if (replaced_bs->blk && target->blk) {
> +error_setg(errp, "Can't create node with two BlockBackends");
> +return;
> +}
> +
>  s = block_job_create(driver, bs, speed, cb, opaque, errp);
>  if (!s) {
>  return;
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 02/21] block: Fix reopen with semantically overlapping options

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> This fixes bdrv_reopen() calls like the following one:
> 
> qemu-io -c 'open -o overlap-check.template=all /tmp/test.qcow2' \
> -c 'reopen -o overlap-check=none'
> 
> The approach taken so far would result in an options QDict that has both
> "overlap-check.template=all" and "overlap-check=none", which obviously
> conflicts. In this case, the old option should be overridden by the
> newly specified option.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 04/21] block: Allow references for backing files

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> For bs->file, using references to existing BDSes has been possible for a
> while already. This patch enables the same for bs->backing_hd.

It's just "backing" now (instead of "backing_hd").

> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 47 +--
>  block/mirror.c|  2 +-
>  include/block/block.h |  3 ++-
>  3 files changed, 32 insertions(+), 20 deletions(-)

An additional thing I believe this patch can do (or a follow-up) is to
remove the part of bdrv_open_inherit() which sets BDRV_O_NO_BACKING if
the @backing option is the empty string.

With the changes done to bdrv_open_backing_file() here, we can easily
put that case into that function (where I think it belongs). If
!reference[0], we can just return success without having actually opened
a backing file.

We may have to think about the case of
{'backing': '', 'backing.driver': 'null-co'}, though. Right now, that is
an error (with a rather obscure error message, probably), and if we want
to keep it an error, we'd have to check whether options is empty in
bdrv_open_backing_file() if !reference[0], and emit a nice error if it
is not ("cannot specify backing options for suppressed backing file" or
something like that).

> diff --git a/block.c b/block.c
> index 675e5a8..ca6c4e9 100644
> --- a/block.c
> +++ b/block.c
> @@ -1182,30 +1182,43 @@ out:
>  /*
>   * Opens the backing file for a BlockDriverState if not yet open
>   *
> - * options is a QDict of options to pass to the block drivers, or NULL for an
> - * empty set of options. The reference to the QDict is transferred to this
> - * function (even on failure), so if the caller intends to reuse the 
> dictionary,
> - * it needs to use QINCREF() before calling bdrv_file_open.
> + * bdref_key specifies the key for the image's BlockdevRef in the options 
> QDict.
> + * That QDict has to be flattened; therefore, if the BlockdevRef is a QDict
> + * itself, all options starting with "${bdref_key}." are considered part of 
> the
> + * BlockdevRef.
> + *
> + * TODO Can this be unified with bdrv_open_image()?
>   */
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error 
> **errp)
> +int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
> +   const char *bdref_key, Error **errp)
>  {
>  char *backing_filename = g_malloc0(PATH_MAX);
> +char *bdref_key_dot;
> +const char *reference = NULL;
>  int ret = 0;
>  BlockDriverState *backing_hd;
> +QDict *options;
> +QDict *tmp_parent_options = NULL;
>  Error *local_err = NULL;
>  
>  if (bs->backing != NULL) {
> -QDECREF(options);
>  goto free_exit;
>  }
>  
>  /* NULL means an empty set of options */
> -if (options == NULL) {
> -options = qdict_new();
> +if (parent_options == NULL) {
> +tmp_parent_options = qdict_new();
> +parent_options = tmp_parent_options;
>  }
>  
>  bs->open_flags &= ~BDRV_O_NO_BACKING;
> -if (qdict_haskey(options, "file.filename")) {
> +
> +bdref_key_dot = g_strdup_printf("%s.", bdref_key);
> +qdict_extract_subqdict(parent_options, , bdref_key_dot);
> +g_free(bdref_key_dot);
> +
> +reference = qdict_get_try_str(parent_options, bdref_key);
> +if (reference || qdict_haskey(options, "file.filename")) {
>  backing_filename[0] = '\0';
>  } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
>  QDECREF(options);
> @@ -1228,19 +1241,17 @@ int bdrv_open_backing_file(BlockDriverState *bs, 
> QDict *options, Error **errp)
>  goto free_exit;
>  }
>  
> -backing_hd = bdrv_new();
> -
>  if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
>  qdict_put(options, "driver", qstring_from_str(bs->backing_format));
>  }
>  
>  assert(bs->backing == NULL);

This assert() can be dropped along with the bdrv_new() call (they are
related: originally, the bdrv_open_inherit() call below took
>backing_hd as its first parameter, and when that was changed, this
assert() was apparently overlooked).

(http://lists.nongnu.org/archive/html/qemu-block/2015-11/msg00344.html)


Anyway, with the commit message fixed:

Reviewed-by: Max Reitz 

> +backing_hd = NULL;
>  ret = bdrv_open_inherit(_hd,
>  *backing_filename ? backing_filename : NULL,
> -NULL, options, 0, bs, _backing, 
> _err);
> +reference, options, 0, bs, _backing,
> +_err);
>  if (ret < 0) {
> -bdrv_unref(backing_hd);
> -backing_hd = NULL;
>  bs->open_flags |= BDRV_O_NO_BACKING;
>  error_setg(errp, "Could not open backing file: %s",
> error_get_pretty(local_err));
> @@ -1253,8 +1264,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, 

Re: [Qemu-block] [PATCH v2 11/21] block: Add infrastructure for option inheritance

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> Options are not actually inherited from the parent node yet, but this
> commit lays the grounds for doing so.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 52 
> ---
>  include/block/block_int.h |  3 ++-
>  2 files changed, 33 insertions(+), 22 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Nov 25, 2015, at 11:23 PM, Eric Blake wrote:

> On 11/25/2015 09:10 PM, Programmingkid wrote:
>> Mac OS X can be picky when it comes to allowing the user
>> to use physical devices in QEMU. Most mounted volumes
>> appear to be off limits to QEMU. If an issue is detected,
>> a message is displayed showing the user how to unmount a
>> volume.
>> 
>> Signed-off-by: John Arbuckle 
>> 
>> ---
>> Added DVD support - real DVD media can now be used in QEMU!
>> Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia.
>> Added mediaType parameter to FindEjectableOpticalMedia() for media 
>> indentification.
>> Removed unneeded FindEjectableCDMedia() prototype.
>> FindEjectableOpticalMedia() now searches for both DVD's and CD's.
>> 
> 
>> +++ b/block/raw-posix.c
>> @@ -42,9 +42,9 @@
>> #include 
>> #include 
>> #include 
>> -//#include 
>> +#include 
>> #include 
>> -#endif
>> +#endif /* (__APPLE__) && (__MACH__) */
>> 
> 
> I don't know if my review of v8 crossed your posting of this patch, but
> I suggested that this hunk belongs in its own patch.

It is now a related change that the code in the patch depends on.

> 
>> #ifdef __sun__
>> #define _POSIX_PTHREAD_SEMANTICS 1
>> @@ -1975,32 +1975,44 @@ BlockDriver bdrv_file = {
>> /* host device */
>> 
>> #if defined(__APPLE__) && defined(__MACH__)
>> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
>> static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
>> CFIndex maxPathSize, int flags);
>> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> +char 
>> *mediaType)
> 
> Unusual indentation; more typical is:
> 
> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
> *mediaIterator,
> | char *mediatType)

I agree. I wanted the second long to be right justified with the 80 character 
line count.

> 
> 
>> +int index;
>> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
>> +classesToMatch = IOServiceMatching(matching_array[index]);
>> +if (classesToMatch == NULL) {
>> +printf("IOServiceMatching returned a NULL dictionary.\n");
> 
> Leftover debugging?

It is actually how the function was originally written. It probably needs to be 
improved.
Should I replace printf() with error_report()?

> 
>> +} else {
>> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
> 
> Missing indentation.

Fixed in the next patch.

> 
>> +
>> kCFBooleanTrue);
>> +}
>> +kernResult = IOServiceGetMatchingServices(masterPort, 
>> classesToMatch,
>> + 
>> mediaIterator);
> 
> More unusual indentation.

Fixed in the next patch. 

> 
>> +if (KERN_SUCCESS != kernResult) {
>> +printf("IOServiceGetMatchingServices returned %d\n", 
>> kernResult);
>> +}
>> 
>> +/* If you found a match, leave the loop */
>> +if (*mediaIterator != 0) {
>> +DPRINTF("Matching using %s\n", matching_array[index]);
>> +snprintf(mediaType, strlen(matching_array[index])+1, "%s",
> 
> Spaces around binary '+'.

What's wrong with no spaces around the plus sign?

> 
>> +/* if a working partition on the device was not found */
>> +if (partition_found == false) {
>> +error_setg(errp, "Error: Failed to find a working partition on "
>> + 
>> "disc!\n");
> 
> and I already pointed out on v8 that this is not the correct usage of
> error_setg().  So, here's hoping v10 addresses the comments here and
> elsewhere.

Kevin Wolf wanted it this way. What would you do instead?

> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Thank you very much for reviewing my patches.

[Qemu-block] [PATCH 08/15] nbd: make server compliant with fixed newstyle spec

[Daniel P. Berrange]

If the client does not request the fixed new style protocol,
then we should only accept NBD_OPT_EXPORT_NAME. All other
options are only valid when fixed new style has been activated.

The qemu-nbd client doesn't currently request fixed new style
protocol, but this change won't break qemu-nbd, because it
fortunately only ever uses NBD_OPT_EXPORT_NAME, so was never
triggering the non-compliant server behaviour.

Signed-off-by: Daniel P. Berrange 
---
 nbd.c | 68 ---
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/nbd.c b/nbd.c
index bdfc45e..09a32a9 100644
--- a/nbd.c
+++ b/nbd.c
@@ -486,6 +486,7 @@ static int nbd_receive_options(NBDClient *client)
 {
 QIOChannel *ioc = client->ioc;
 uint32_t flags;
+bool fixedNewstyle = false;
 
 /* Client sends:
 [ 0 ..   3]   client flags
@@ -507,14 +508,19 @@ static int nbd_receive_options(NBDClient *client)
 }
 TRACE("Checking client flags");
 be32_to_cpus();
-if (flags != 0 && flags != NBD_FLAG_C_FIXED_NEWSTYLE) {
-LOG("Bad client flags received");
+if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
+TRACE("Support supports fixed newstyle handshake");
+fixedNewstyle = true;
+flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
+}
+if (flags != 0) {
+TRACE("Unknown client flags 0x%x received", flags);
 return -EIO;
 }
 
 while (1) {
 int ret;
-uint32_t tmp, length;
+uint32_t clientflags, length;
 uint64_t magic;
 
 if (read_sync(ioc, , sizeof(magic)) != sizeof(magic)) {
@@ -527,10 +533,12 @@ static int nbd_receive_options(NBDClient *client)
 return -EINVAL;
 }
 
-if (read_sync(ioc, , sizeof(tmp)) != sizeof(tmp)) {
+if (read_sync(ioc, , sizeof(clientflags)) !=
+sizeof(clientflags)) {
 LOG("read failed");
 return -EINVAL;
 }
+clientflags = be32_to_cpu(clientflags);
 
 if (read_sync(ioc, , sizeof(length)) != sizeof(length)) {
 LOG("read failed");
@@ -538,26 +546,40 @@ static int nbd_receive_options(NBDClient *client)
 }
 length = be32_to_cpu(length);
 
-TRACE("Checking option");
-switch (be32_to_cpu(tmp)) {
-case NBD_OPT_LIST:
-ret = nbd_handle_list(client, length);
-if (ret < 0) {
-return ret;
+TRACE("Checking option 0x%x", clientflags);
+if (fixedNewstyle) {
+switch (clientflags) {
+case NBD_OPT_LIST:
+ret = nbd_handle_list(client, length);
+if (ret < 0) {
+return ret;
+}
+break;
+
+case NBD_OPT_ABORT:
+return -EINVAL;
+
+case NBD_OPT_EXPORT_NAME:
+return nbd_handle_export_name(client, length);
+
+default:
+TRACE("Unsupported option 0x%x", clientflags);
+nbd_send_rep(client->ioc, NBD_REP_ERR_UNSUP, clientflags);
+return -EINVAL;
+}
+} else {
+/*
+ * If broken new-style we should drop the connection
+ * for anything except NBD_OPT_EXPORT_NAME
+ */
+switch (clientflags) {
+case NBD_OPT_EXPORT_NAME:
+return nbd_handle_export_name(client, length);
+
+default:
+TRACE("Unsupported option 0x%x", clientflags);
+return -EINVAL;
 }
-break;
-
-case NBD_OPT_ABORT:
-return -EINVAL;
-
-case NBD_OPT_EXPORT_NAME:
-return nbd_handle_export_name(client, length);
-
-default:
-tmp = be32_to_cpu(tmp);
-LOG("Unsupported option 0x%x", tmp);
-nbd_send_rep(client->ioc, NBD_REP_ERR_UNSUP, tmp);
-return -EINVAL;
 }
 }
 }
-- 
2.5.0

[Qemu-block] [PATCH 10/15] nbd: allow setting of an export name for qemu-nbd server

[Daniel P. Berrange]

The qemu-nbd server currently always uses the old style protocol
since it never sets any export name. This is a problem because
future TLS support will require use of the new style protocol
negotiation.

This adds a "--exportname NAME" argument to qemu-nbd which allows
the user to set an explicit export name. When --exportname is
set the server will always use the new style protocol.

Signed-off-by: Daniel P. Berrange 
---
 qemu-nbd.c| 14 --
 qemu-nbd.texi |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 42ccf05..3f7f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -47,6 +47,7 @@
 #define QEMU_NBD_OPT_OBJECT5
 
 static NBDExport *exp;
+static bool newproto;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -341,7 +342,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition 
cond, gpointer opaque)
 return TRUE;
 }
 
-if (nbd_client_new(exp, cioc, nbd_client_closed)) {
+if (nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed)) {
 nb_fds++;
 nbd_update_server_watch();
 }
@@ -469,7 +470,7 @@ int main(int argc, char **argv)
 off_t fd_size;
 QemuOpts *sn_opts = NULL;
 const char *sn_id_or_name = NULL;
-const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
+const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
 struct option lopt[] = {
 { "help", 0, NULL, 'h' },
 { "version", 0, NULL, 'V' },
@@ -493,6 +494,7 @@ int main(int argc, char **argv)
 { "persistent", 0, NULL, 't' },
 { "verbose", 0, NULL, 'v' },
 { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
+{ "exportname", 1, NULL, 'x' },
 { NULL, 0, NULL, 0 }
 };
 int ch;
@@ -510,6 +512,7 @@ int main(int argc, char **argv)
 BlockdevDetectZeroesOptions detect_zeroes = 
BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
 QDict *options = NULL;
 QemuOpts *opts;
+const char *exportname = NULL;
 
 /* The client thread uses SIGTERM to interrupt the server.  A signal
  * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -645,6 +648,9 @@ int main(int argc, char **argv)
 case 't':
 persistent = 1;
 break;
+case 'x':
+exportname = optarg;
+break;
 case 'v':
 verbose = 1;
 break;
@@ -812,6 +818,10 @@ int main(int argc, char **argv)
 if (!exp) {
 errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
 }
+if (exportname) {
+nbd_export_set_name(exp, exportname);
+newproto = true;
+}
 
 server_ioc = qio_channel_socket_new();
 if (qio_channel_socket_listen_sync(server_ioc, saddr, _err) < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 3b2004e..6ff3920 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -64,6 +64,9 @@ Export QEMU disk image using NBD protocol.
   force block driver for format @var{fmt} instead of auto-detecting
 @item -t, --persistent
   don't exit on the last connection
+@item -x NAME, --exportname=NAME
+  set the NDB volume export name. This switches the server to use
+  the new style NBD protocol negotiation
 @item -v, --verbose
   display extra debugging information
 @item -h, --help
-- 
2.5.0

[Qemu-block] [PATCH 11/15] nbd: pick first exported volume if no export name is requested

[Daniel P. Berrange]

The NBD client is currently only capable of using the new style
protocol negotiation if an explicit export name is provided.
This is a problem, because TLS support will require use of the
new style protocol in all cases, and we wish to keep the export
name as an optional request for backwards compatibility.

The trivial way to deal with this is to use the NBD protocol
option for listing exports and then pick the first returned
export as the one to use. This makes the client "do the right
thing" in the normal case where the qemu-nbd server only
exports a single volume.

Furthermore, in order to get proper error reporting of unknown
options with fixed new style protocol, we must not send the
NBD_OPT_EXPORT_NAME as the first thing. Thus, even if an export
name is provided we still send NBD_OPT_LIST to enumerate server
exports. This also gives us clearer error reporting in the case
that the requested export name does not exist. If NBD_OPT_LIST
is not supported, we still fallback to using the specified
export name, so as not to break compatibility with old QEMU
NBD server impls that predated NBD_OPT_LIST

Signed-off-by: Daniel P. Berrange 
---
 nbd.c | 202 --
 1 file changed, 197 insertions(+), 5 deletions(-)

diff --git a/nbd.c b/nbd.c
index 6ec04a7..0f6c755 100644
--- a/nbd.c
+++ b/nbd.c
@@ -468,6 +468,7 @@ static int nbd_handle_export_name(NBDClient *client, 
uint32_t length)
 goto fail;
 }
 name[length] = '\0';
+TRACE("Client requested export '%s'", name);
 
 client->exp = nbd_export_find(name);
 if (!client->exp) {
@@ -659,12 +660,187 @@ fail:
 return rc;
 }
 
+
+static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
+{
+if (!(type & (1 << 31))) {
+return 0;
+}
+
+switch (type) {
+case NBD_REP_ERR_UNSUP:
+error_setg(errp, "Unsupported option type %x", opt);
+break;
+
+case NBD_REP_ERR_INVALID:
+error_setg(errp, "Invalid data length for option %x", opt);
+break;
+
+default:
+error_setg(errp, "Unknown error code when asking for option %x", opt);
+break;
+}
+
+return -1;
+}
+
+static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
+{
+uint64_t magic;
+uint32_t opt;
+uint32_t type;
+uint32_t len;
+uint32_t namelen;
+
+*name = NULL;
+if (read_sync(ioc, , sizeof(magic)) != sizeof(magic)) {
+error_setg(errp, "failed to read list option magic");
+return -1;
+}
+magic = be64_to_cpu(magic);
+if (magic != NBD_REP_MAGIC) {
+error_setg(errp, "Unexpected option list magic");
+return -1;
+}
+if (read_sync(ioc, , sizeof(opt)) != sizeof(opt)) {
+error_setg(errp, "failed to read list option");
+return -1;
+}
+opt = be32_to_cpu(opt);
+if (opt != NBD_OPT_LIST) {
+error_setg(errp, "Unexpected option type %x expected %x",
+   opt, NBD_OPT_LIST);
+return -1;
+}
+
+if (read_sync(ioc, , sizeof(type)) != sizeof(type)) {
+error_setg(errp, "failed to read list option type");
+return -1;
+}
+type = be32_to_cpu(type);
+if (type == NBD_REP_ERR_UNSUP) {
+return 0;
+}
+if (nbd_handle_reply_err(opt, type, errp) < 0) {
+return -1;
+}
+
+if (read_sync(ioc, , sizeof(len)) != sizeof(len)) {
+error_setg(errp, "failed to read option length");
+return -1;
+}
+len = be32_to_cpu(len);
+
+if (type == NBD_REP_ACK) {
+if (len != 0) {
+error_setg(errp, "length too long for option end");
+return -1;
+}
+} else if (type == NBD_REP_SERVER) {
+if (read_sync(ioc, , sizeof(namelen)) != sizeof(namelen)) {
+error_setg(errp, "failed to read option name length");
+return -1;
+}
+namelen = be32_to_cpu(namelen);
+if (len != (namelen + sizeof(namelen))) {
+error_setg(errp, "incorrect option mame length");
+return -1;
+}
+if (namelen > 255) {
+error_setg(errp, "export name length too long %d", namelen);
+return -1;
+}
+
+*name = g_new0(char, namelen + 1);
+if (read_sync(ioc, *name, namelen) != namelen) {
+error_setg(errp, "failed to read export name");
+g_free(*name);
+*name = NULL;
+return -1;
+}
+(*name)[namelen] = '\0';
+} else {
+error_setg(errp, "Unexpected reply type %x expected %x",
+   type, NBD_REP_SERVER);
+return -1;
+}
+return 1;
+}
+
+
+static char *nbd_receive_pick_export(QIOChannel *ioc,
+ const char *wantname,
+ Error **errp)
+{
+uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
+uint32_t opt = 

[Qemu-block] [PATCH 12/15] nbd: implement TLS support in the protocol negotiation

[Daniel P. Berrange]

This extends the NBD protocol handling code so that it is capable
of negotiating TLS support during the connection setup. This involves
requesting the STARTTLS protocol option before any other NBD options.

Signed-off-by: Daniel P. Berrange 
---
 block/nbd-client.c  |  12 ++-
 blockdev-nbd.c  |   2 +-
 include/block/nbd.h |   7 ++
 nbd.c   | 284 +---
 qemu-nbd.c  |   4 +-
 5 files changed, 290 insertions(+), 19 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index ed69d4b..f7bacfd 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -389,7 +389,10 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket 
*sioc,
 qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
 ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
->nbdflags, >size, errp);
+>nbdflags,
+NULL, NULL,
+>ioc,
+>size, errp);
 if (ret < 0) {
 logout("Failed to negotiate with the NBD server\n");
 return ret;
@@ -399,8 +402,11 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket 
*sioc,
 qemu_co_mutex_init(>free_sema);
 client->sioc = sioc;
 object_ref(OBJECT(client->sioc));
-client->ioc = QIO_CHANNEL(sioc);
-object_ref(OBJECT(client->ioc));
+
+if (!client->ioc) {
+client->ioc = QIO_CHANNEL(sioc);
+object_ref(OBJECT(client->ioc));
+}
 
 /* Now that we're connected, set the socket to be non-blocking and
  * kick the reply mechanism.  */
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 7d4c415..6af1684 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -33,7 +33,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition 
condition,
 return TRUE;
 }
 
-nbd_client_new(NULL, cioc, nbd_client_put);
+nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
 object_unref(OBJECT(cioc));
 return TRUE;
 }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 0230c7f..502edfe 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "io/channel-socket.h"
+#include "crypto/tlscreds.h"
 
 struct nbd_request {
 uint32_t magic;
@@ -57,6 +58,8 @@ struct nbd_reply {
 #define NBD_REP_SERVER  (2) /* Export description. */
 #define NBD_REP_ERR_UNSUP   ((UINT32_C(1) << 31) | 1) /* Unknown option. */
 #define NBD_REP_ERR_INVALID ((UINT32_C(1) << 31) | 3) /* Invalid length. */
+#define NBD_REP_ERR_TLS_REQD((UINT32_C(1) << 31) | 5) /* TLS required */
+
 
 #define NBD_CMD_MASK_COMMAND   0x
 #define NBD_CMD_FLAG_FUA   (1 << 16)
@@ -81,6 +84,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
  size_t length,
  bool do_read);
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+  QCryptoTLSCreds *tlscreds, const char *hostname,
+  QIOChannel **outioc,
   off_t *size, Error **errp);
 int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
 ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
@@ -106,6 +111,8 @@ void nbd_export_close_all(void);
 
 NBDClient *nbd_client_new(NBDExport *exp,
   QIOChannelSocket *sioc,
+  QCryptoTLSCreds *tlscreds,
+  const char *tlsaclname,
   void (*close)(NBDClient *));
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
diff --git a/nbd.c b/nbd.c
index 0f6c755..f17500e 100644
--- a/nbd.c
+++ b/nbd.c
@@ -18,6 +18,7 @@
 
 #include "block/nbd.h"
 #include "sysemu/block-backend.h"
+#include "io/channel-tls.h"
 
 #include "qemu/coroutine.h"
 #include "qemu/iov.h"
@@ -85,6 +86,8 @@
 #define NBD_OPT_EXPORT_NAME (1)
 #define NBD_OPT_ABORT   (2)
 #define NBD_OPT_LIST(3)
+#define NBD_OPT_PEEK_EXPORT (4)
+#define NBD_OPT_STARTTLS(5)
 
 /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
  * but only a limited set of errno values is specified in the protocol.
@@ -171,6 +174,8 @@ struct NBDClient {
 void (*close)(NBDClient *client);
 
 NBDExport *exp;
+QCryptoTLSCreds *tlscreds;
+char *tlsaclname;
 QIOChannelSocket *sioc; /* The underlying data channel */
 QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
 
@@ -365,6 +370,8 @@ static int nbd_send_rep(QIOChannel *ioc, uint32_t type, 
uint32_t opt)
 uint64_t magic;
 uint32_t len;
 
+TRACE("Reply opt=%x type=%x", type, opt);
+
 magic = cpu_to_be64(NBD_REP_MAGIC);
 if (write_sync(ioc, , sizeof(magic)) != sizeof(magic)) {
 LOG("write failed (rep magic)");

[Qemu-block] [PATCH 06/15] nbd: convert to using I/O channels for actual socket I/O

[Daniel P. Berrange]

Now that all callers are converted to use I/O channels for
initial connection setup, it is possible to switch the core
NBD protocol handling core over to use QIOChannel APIs for
acutal sockets I/O.

Signed-off-by: Daniel P. Berrange 
---
 block/nbd-client.c  |  19 ++--
 blockdev-nbd.c  |   7 +-
 include/block/nbd.h |  19 ++--
 nbd.c   | 289 
 qemu-nbd.c  |   9 +-
 5 files changed, 205 insertions(+), 138 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index bf5b0a3..ed69d4b 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -71,7 +71,7 @@ static void nbd_reply_ready(void *opaque)
  * that another thread has done the same thing in parallel, so
  * the socket is not readable anymore.
  */
-ret = nbd_receive_reply(s->sioc->fd, >reply);
+ret = nbd_receive_reply(s->ioc, >reply);
 if (ret == -EAGAIN) {
 return;
 }
@@ -122,6 +122,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
 }
 }
 
+g_assert(qemu_in_coroutine());
 assert(i < MAX_NBD_REQUESTS);
 request->handle = INDEX_TO_HANDLE(s, i);
 s->send_coroutine = qemu_coroutine_self();
@@ -131,17 +132,17 @@ static int nbd_co_send_request(BlockDriverState *bs,
nbd_reply_ready, nbd_restart_write, bs);
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
-rc = nbd_send_request(s->sioc->fd, request);
+rc = nbd_send_request(s->ioc, request);
 if (rc >= 0) {
-ret = qemu_co_sendv(s->sioc->fd, qiov->iov, qiov->niov,
-offset, request->len);
+ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+   offset, request->len, 0);
 if (ret != request->len) {
 rc = -EIO;
 }
 }
 qio_channel_set_cork(s->ioc, false);
 } else {
-rc = nbd_send_request(s->sioc->fd, request);
+rc = nbd_send_request(s->ioc, request);
 }
 aio_set_fd_handler(aio_context, s->sioc->fd, false,
nbd_reply_ready, NULL, bs);
@@ -164,8 +165,8 @@ static void nbd_co_receive_reply(NbdClientSession *s,
 reply->error = EIO;
 } else {
 if (qiov && reply->error == 0) {
-ret = qemu_co_recvv(s->sioc->fd, qiov->iov, qiov->niov,
-offset, request->len);
+ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+   offset, request->len, 1);
 if (ret != request->len) {
 reply->error = EIO;
 }
@@ -372,7 +373,7 @@ void nbd_client_close(BlockDriverState *bs)
 return;
 }
 
-nbd_send_request(client->sioc->fd, );
+nbd_send_request(client->ioc, );
 
 nbd_teardown_connection(bs);
 }
@@ -387,7 +388,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket 
*sioc,
 logout("session init %s\n", export);
 qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
-ret = nbd_receive_negotiate(sioc->fd, export,
+ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
 >nbdflags, >size, errp);
 if (ret < 0) {
 logout("Failed to negotiate with the NBD server\n");
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 71ee021..7d4c415 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -26,7 +26,6 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition 
condition,
gpointer opaque)
 {
 QIOChannelSocket *cioc;
-int fd;
 
 cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
  NULL);
@@ -34,11 +33,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition 
condition,
 return TRUE;
 }
 
-fd = dup(cioc->fd);
-if (fd >= 0 &&
-!nbd_client_new(NULL, fd, nbd_client_put)) {
-close(fd);
-}
+nbd_client_new(NULL, cioc, nbd_client_put);
 object_unref(OBJECT(cioc));
 return TRUE;
 }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65f409d..0230c7f 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -23,6 +23,7 @@
 
 #include "qemu-common.h"
 #include "qemu/option.h"
+#include "io/channel-socket.h"
 
 struct nbd_request {
 uint32_t magic;
@@ -73,12 +74,17 @@ enum {
 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
 
-ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
+ssize_t nbd_wr_syncv(QIOChannel *ioc,
+ struct iovec *iov,
+ size_t niov,
+ size_t offset,
+ size_t length,
+ bool do_read);
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
   

[Qemu-block] [PATCH 09/15] nbd: make client request fixed new style if advertized

[Daniel P. Berrange]

If the server advertizes support for the fixed new style
negotiation, the client should in turn enable new style.
This will allow the client to negotiate further NBD
options besides the export name.

Signed-off-by: Daniel P. Berrange 
---
 nbd.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/nbd.c b/nbd.c
index 09a32a9..6ec04a7 100644
--- a/nbd.c
+++ b/nbd.c
@@ -664,7 +664,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
 {
 char buf[256];
 uint64_t magic, s;
-uint16_t tmp;
 int rc;
 
 TRACE("Receiving negotiation.");
@@ -705,19 +704,26 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
 TRACE("Magic is 0x%" PRIx64, magic);
 
 if (magic == NBD_OPTS_MAGIC) {
-uint32_t reserved = 0;
+uint32_t clientflags = 0;
 uint32_t opt;
 uint32_t namesize;
+uint16_t globalflags;
+uint16_t exportflags;
 
-if (read_sync(ioc, , sizeof(tmp)) != sizeof(tmp)) {
+if (read_sync(ioc, , sizeof(globalflags)) !=
+sizeof(globalflags)) {
 error_setg(errp, "Failed to read server flags");
 goto fail;
 }
-*flags = be16_to_cpu(tmp) << 16;
-/* reserved for future use */
-if (write_sync(ioc, , sizeof(reserved)) !=
-sizeof(reserved)) {
-error_setg(errp, "Failed to read reserved field");
+*flags = be16_to_cpu(globalflags) << 16;
+if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+TRACE("Server supports fixed new style");
+clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
+}
+/* client requested flags */
+if (write_sync(ioc, , sizeof(clientflags)) !=
+sizeof(clientflags)) {
+error_setg(errp, "Failed to send clientflags field");
 goto fail;
 }
 /* write the export name */
@@ -753,11 +759,12 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
 *size = be64_to_cpu(s);
 TRACE("Size is %" PRIu64, *size);
 
-if (read_sync(ioc, , sizeof(tmp)) != sizeof(tmp)) {
+if (read_sync(ioc, , sizeof(exportflags)) !=
+sizeof(exportflags)) {
 error_setg(errp, "Failed to read export flags");
 goto fail;
 }
-*flags |= be16_to_cpu(tmp);
+*flags |= be16_to_cpu(exportflags);
 } else if (magic == NBD_CLIENT_MAGIC) {
 if (name) {
 error_setg(errp, "Server does not support export names");
-- 
2.5.0

[Qemu-block] [PATCH 13/15] nbd: enable use of TLS with NBD block driver

[Daniel P. Berrange]

This modifies the NBD driver so that it is possible to request
use of TLS. This is done by providing the 'tls-creds' parameter
with the ID of a previously created QCryptoTLSCreds object.

For example

  $QEMU -object tls-creds-x509,id=tls0,endpoint=client,\
dir=/home/berrange/security/qemutls \
-drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

The client will drop the connection if the NBD server does not
provide TLS.

Signed-off-by: Daniel P. Berrange 
---
 block/nbd-client.c | 10 ---
 block/nbd-client.h |  2 ++
 block/nbd.c| 78 +++---
 3 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index f7bacfd..024d5f3 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -378,8 +378,12 @@ void nbd_client_close(BlockDriverState *bs)
 nbd_teardown_connection(bs);
 }
 
-int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
-const char *export, Error **errp)
+int nbd_client_init(BlockDriverState *bs,
+QIOChannelSocket *sioc,
+const char *export,
+QCryptoTLSCreds *tlscreds,
+const char *hostname,
+Error **errp)
 {
 NbdClientSession *client = nbd_get_client_session(bs);
 int ret;
@@ -390,7 +394,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket 
*sioc,
 
 ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
 >nbdflags,
-NULL, NULL,
+tlscreds, hostname,
 >ioc,
 >size, errp);
 if (ret < 0) {
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e8b3283..53f116d 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -39,6 +39,8 @@ NbdClientSession *nbd_get_client_session(BlockDriverState 
*bs);
 int nbd_client_init(BlockDriverState *bs,
 QIOChannelSocket *sock,
 const char *export_name,
+QCryptoTLSCreds *tlscreds,
+const char *hostname,
 Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
diff --git a/block/nbd.c b/block/nbd.c
index e51acfc..5f7061e 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -259,36 +259,92 @@ static QIOChannelSocket 
*nbd_establish_connection(SocketAddress *saddr,
 return sioc;
 }
 
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+Object *obj;
+QCryptoTLSCreds *creds;
+
+obj = object_resolve_path_component(
+object_get_objects_root(), id);
+if (!obj) {
+error_setg(errp, "No TLS credentials with id '%s'",
+   id);
+return NULL;
+}
+creds = (QCryptoTLSCreds *)
+object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+if (!creds) {
+error_setg(errp, "Object with id '%s' is not TLS credentials",
+   id);
+return NULL;
+}
+
+if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
+error_setg(errp,
+   "Expecting TLS credentials with a client endpoint");
+return NULL;
+}
+object_ref(obj);
+return creds;
+}
+
+
 static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
 BDRVNBDState *s = bs->opaque;
 char *export = NULL;
-int result;
-QIOChannelSocket *sioc;
+QIOChannelSocket *sioc = NULL;
 SocketAddress *saddr;
+const char *tlscredsid;
+QCryptoTLSCreds *tlscreds = NULL;
+const char *hostname = NULL;
+int ret = -EINVAL;
 
 /* Pop the config into our state object. Exit if invalid. */
 saddr = nbd_config(s, options, , errp);
 if (!saddr) {
-return -EINVAL;
+goto error;
+}
+
+tlscredsid = g_strdup(qdict_get_try_str(options, "tls-creds"));
+if (tlscredsid) {
+qdict_del(options, "tls-creds");
+tlscreds = nbd_get_tls_creds(tlscredsid, errp);
+if (!tlscreds) {
+goto error;
+}
+
+if (saddr->type != SOCKET_ADDRESS_KIND_INET) {
+error_setg(errp, "TLS only supported over IP sockets");
+goto error;
+}
+hostname = saddr->u.inet->host;
 }
 
 /* establish TCP connection, return error if it fails
  * TODO: Configurable retry-until-timeout behaviour.
  */
 sioc = nbd_establish_connection(saddr, errp);
-qapi_free_SocketAddress(saddr);
 if (!sioc) {
-g_free(export);
-return -ECONNREFUSED;
+ret = -ECONNREFUSED;
+goto error;
 }
 
 /* NBD handshake */
-result = nbd_client_init(bs, sioc, export, errp);
-object_unref(OBJECT(sioc));
+ret = nbd_client_init(bs, sioc, export,
+  tlscreds, hostname, errp);
+ error:
+

[Qemu-block] [PATCH 15/15] nbd: enable use of TLS with nbd-server-start command

[Daniel P. Berrange]

This modifies the nbd-server-start QMP command so that it
is possible to request use of TLS. This is done by adding
a new optional parameter "tls-creds" which provides the ID
of a previously created QCryptoTLSCreds object instance.

TLS is only supported when using an IPv4/IPv6 socket listener.

Signed-off-by: Daniel P. Berrange 
---
 blockdev-nbd.c  | 122 ++--
 hmp.c   |   2 +-
 qapi/block.json |   4 +-
 qmp-commands.hx |   2 +-
 4 files changed, 105 insertions(+), 25 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 6af1684..e287f05 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -19,42 +19,126 @@
 #include "block/nbd.h"
 #include "io/channel-socket.h"
 
-static QIOChannelSocket *server_ioc;
-static int server_watch = -1;
+typedef struct NBDServerData {
+QIOChannelSocket *listen_ioc;
+int watch;
+QCryptoTLSCreds *tlscreds;
+} NBDServerData;
+
+static NBDServerData *nbd_server;
+
 
 static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
gpointer opaque)
 {
 QIOChannelSocket *cioc;
 
+if (!nbd_server) {
+return FALSE;
+}
+
 cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
  NULL);
 if (!cioc) {
 return TRUE;
 }
 
-nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
+nbd_client_new(NULL, cioc,
+   nbd_server->tlscreds, NULL,
+   nbd_client_put);
 object_unref(OBJECT(cioc));
 return TRUE;
 }
 
-void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
+
+static void nbd_server_free(NBDServerData *server)
 {
-if (server_ioc) {
-error_setg(errp, "NBD server already running");
+if (!server) {
 return;
 }
 
-server_ioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+if (server->watch != -1) {
+g_source_remove(server->watch);
+}
+object_unref(OBJECT(server->listen_ioc));
+if (server->tlscreds) {
+object_unref(OBJECT(server->tlscreds));
+}
+
+g_free(server);
+}
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+Object *obj;
+QCryptoTLSCreds *creds;
+
+obj = object_resolve_path_component(
+object_get_objects_root(), id);
+if (!obj) {
+error_setg(errp, "No TLS credentials with id '%s'",
+   id);
+return NULL;
+}
+creds = (QCryptoTLSCreds *)
+object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+if (!creds) {
+error_setg(errp, "Object with id '%s' is not TLS credentials",
+   id);
+return NULL;
+}
+
+if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+error_setg(errp,
+   "Expecting TLS credentials with a server endpoint");
+return NULL;
+}
+object_ref(obj);
+return creds;
+}
+
+
+void qmp_nbd_server_start(SocketAddress *addr,
+  bool has_tls_creds, const char *tls_creds,
+  Error **errp)
+{
+if (nbd_server) {
+error_setg(errp, "NBD server already running");
 return;
 }
 
-server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
- G_IO_IN,
- nbd_accept,
- NULL,
- NULL);
+nbd_server = g_new0(NBDServerData, 1);
+nbd_server->watch = -1;
+nbd_server->listen_ioc = qio_channel_socket_new();
+if (qio_channel_socket_listen_sync(
+nbd_server->listen_ioc, addr, errp) < 0) {
+goto error;
+}
+
+if (has_tls_creds) {
+nbd_server->tlscreds = nbd_get_tls_creds(tls_creds, errp);
+if (!nbd_server->tlscreds) {
+goto error;
+}
+
+if (addr->type != SOCKET_ADDRESS_KIND_INET) {
+error_setg(errp, "TLS is only supported with IPv4/IPv6");
+goto error;
+}
+}
+
+nbd_server->watch = qio_channel_add_watch(
+QIO_CHANNEL(nbd_server->listen_ioc),
+G_IO_IN,
+nbd_accept,
+NULL,
+NULL);
+
+return;
+
+ error:
+nbd_server_free(nbd_server);
+nbd_server = NULL;
 }
 
 /*
@@ -89,7 +173,7 @@ void qmp_nbd_server_add(const char *device, bool 
has_writable, bool writable,
 NBDExport *exp;
 NBDCloseNotifier *n;
 
-if (!server_ioc) {
+if (!nbd_server) {
 error_setg(errp, "NBD server not running");
 return;
 }
@@ -139,12 +223,6 @@ void qmp_nbd_server_stop(Error **errp)
 nbd_close_notifier(>n, nbd_export_get_blockdev(cn->exp));
 }
 
-if (server_watch != -1) {
-g_source_remove(server_watch);
-server_watch = -1;
-}
-if (server_ioc) {
-object_unref(OBJECT(server_ioc));
-

[Qemu-block] [PATCH 14/15] nbd: enable use of TLS with qemu-nbd server

[Daniel P. Berrange]

This modifies the qemu-nbd program so that it is possible to
request the use of TLS with the server. It simply adds a new
command line option --tls-creds which is used to provide the
ID of a QCryptoTLSCreds object previously created via the
--object command line option.

For example

  qemu-nbd --object tls-creds-x509,id=tls0,endpoint=server,\
dir=/home/berrange/security/qemutls \
   --tls-creds tls0
   --exportname default

Note that it is mandatory to supply the --exportname argument
when requesting TLS, since it requires use of the new style
NBD protocol where the client requests a volume name explicitly.

TLS is only supported when using an IPv4/IPv6 socket listener.
It is not possible to use with UNIX sockets, which includes
when connecting the NBD server to a host device.

Signed-off-by: Daniel P. Berrange 
---
 qemu-nbd.c| 57 -
 qemu-nbd.texi |  4 
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 24cdb65..e579188 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -45,6 +45,7 @@
 #define QEMU_NBD_OPT_DISCARD   3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
 #define QEMU_NBD_OPT_OBJECT5
+#define QEMU_NBD_OPT_TLSCREDS  6
 
 static NBDExport *exp;
 static bool newproto;
@@ -57,6 +58,7 @@ static int shared = 1;
 static int nb_fds;
 static QIOChannelSocket *server_ioc;
 static int server_watch = -1;
+static QCryptoTLSCreds *tlscreds;
 
 static void usage(const char *name)
 {
@@ -344,7 +346,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition 
cond, gpointer opaque)
 }
 
 if (nbd_client_new(newproto ? NULL : exp, cioc,
-   NULL, NULL, nbd_client_closed)) {
+   tlscreds, NULL, nbd_client_closed)) {
 nb_fds++;
 nbd_update_server_watch();
 }
@@ -458,6 +460,37 @@ out:
 return 0;
 }
 
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+Object *obj;
+QCryptoTLSCreds *creds;
+
+obj = object_resolve_path_component(
+object_get_objects_root(), id);
+if (!obj) {
+error_setg(errp, "No TLS credentials with id '%s'",
+   id);
+return NULL;
+}
+creds = (QCryptoTLSCreds *)
+object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+if (!creds) {
+error_setg(errp, "Object with id '%s' is not TLS credentials",
+   id);
+return NULL;
+}
+
+if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+error_setg(errp,
+   "Expecting TLS credentials with a server endpoint");
+return NULL;
+}
+object_ref(obj);
+return creds;
+}
+
+
 int main(int argc, char **argv)
 {
 BlockBackend *blk;
@@ -497,6 +530,7 @@ int main(int argc, char **argv)
 { "verbose", 0, NULL, 'v' },
 { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
 { "exportname", 1, NULL, 'x' },
+{ "tls-creds", 1, NULL, QEMU_NBD_OPT_TLSCREDS },
 { NULL, 0, NULL, 0 }
 };
 int ch;
@@ -515,6 +549,7 @@ int main(int argc, char **argv)
 QDict *options = NULL;
 QemuOpts *opts;
 const char *exportname = NULL;
+const char *tlscredsid = NULL;
 
 /* The client thread uses SIGTERM to interrupt the server.  A signal
  * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -671,6 +706,9 @@ int main(int argc, char **argv)
 exit(1);
 }
 break;
+case QEMU_NBD_OPT_TLSCREDS:
+tlscredsid = optarg;
+break;
 case '?':
 errx(EXIT_FAILURE, "Try `%s --help' for more information.",
  argv[0]);
@@ -689,6 +727,23 @@ int main(int argc, char **argv)
 exit(1);
 }
 
+if (tlscredsid) {
+if (!exportname) {
+errx(EXIT_FAILURE, "Export name is required when using TLS");
+}
+if (sockpath) {
+errx(EXIT_FAILURE, "TLS is only supported with IPv4/IPv6");
+}
+if (device) {
+errx(EXIT_FAILURE, "TLS is not supported with a host device");
+}
+tlscreds = nbd_get_tls_creds(tlscredsid, _err);
+if (local_err) {
+errx(EXIT_FAILURE, "Failed to get TLS creds %s",
+ error_get_pretty(local_err));
+}
+}
+
 if (disconnect) {
 int nbdfd = open(argv[optind], O_RDWR);
 if (nbdfd < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 6ff3920..a12d2e1 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -67,6 +67,10 @@ Export QEMU disk image using NBD protocol.
 @item -x NAME, --exportname=NAME
   set the NDB volume export name. This switches the server to use
   the new style NBD protocol negotiation
+@item --tls-creds=ID
+  enable mandatory TLS encryption for the server by setting the ID
+  of the TLS credentials object previously created with the 

[Qemu-block] [PATCH 00/15] Implement TLS support to QEMU NBD server & client

[Daniel P. Berrange]

This series of patches implements support for TLS in the QEMU NBD
server and client code.

It is implementing the NBD_OPT_STARTTLS option that was previously
discussed here:

  https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html

And is also described in the NBD spec here:

  https://github.com/yoe/nbd/blob/master/doc/proto.md

Based on my impl I think the NBD_OPT_STARTTLS specification is
fine. In my impl, when TLS is requested in the server, I chose
to *refuse* all NBD options until NBD_OPT_STARTTLS has completed
successfully. There is an annoying thing about the fixed new style
protocol in that it made an exception for NBD_OPT_EXPORT_NAME,
so that it is still not possible to report errors for that option.

[qupte]
 - The server will reply to any option apart from `NBD_OPT_EXPORT_NAME`
with reply packets in the following format:

  S: 64 bits, `0x3e889045565a9` (magic number for replies)  
  S: 32 bits, the option as sent by the client to which this is a reply  
  S: 32 bits, reply type (e.g., `NBD_REP_ACK` for successful completion,
 or `NBD_REP_ERR_UNSUP` to mark use of an option not known by this
 server  
  S: 32 bits, length of the reply. This may be zero for some replies, in
 which case the next field is not sent  
  S: any data as required by the reply (e.g., an export name in the case
 of `NBD_REP_SERVER`)
[/quote]

I wish those words "apart from NBD_OPT_EXPORT_NAME" were not there
for fixed new style protocol - maybe it needs a very-fixed new
style protocol...

As is, if the client connects to a TLS enabled NBD server and then
immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us
to send back NBD_REP_ERR_TLS_REQD as the spec requires that the
server close the connection :-(  For this reason I have made the
qemu NBD client always send NBD_OPT_LIST as the first thing it
does, so that we can see the NBD_REP_ERR_TLS_REQD response.


I chose to *NOT* implement the NBD_OPT_PEEK_EXPORT option as it is
inherently insecure to have a client to ask the server which
exports need TLS, while the protocol is still running in plain text
mode, as it allows a trivial MITM downgrade attack. I can see value
in the NBD_OPT_PEEK_EXPORT option for probing other features, but
only after TLS has been negotiated. As such I believe NBD_F_EXP_TLS_OK
and NBD_F_EXP_TLS_REQ should be deleted from the spec as it is not
possible to use them in a secure manner.

This series of patches has my v3 I/O channels patch series as a
pre-requisite:

  https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04208.html

Usage of TLS is described in the commit messages for each patch,
but for sake of people who don't want to explore the series, here's
the summary

Starting QEMU system emulator with a disk backed by an TLS encrypted
NBD export

  $ qemu-system-x86_64 \
 -object 
tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls
 -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

Starting a standalone NBD server providing a TLS encrypted NBD export

  $ qemu-nbd \
 --object 
tls-creds-x509,id=tls0,endpoint=server,dir=/home/berrange/security/qemutls
 --tls-creds tls0 \
 --exportname default

NB, exportname is mandatory, since TLS requires the fixed-new style
NBD protocol negotiation.

Starting a QEMU system emulator built-in NBD server

  $ qemu-system-x86_64 \
 -qmp unix:/tmp/qmp,server \
 -hda /home/berrange/Fedora-Server-netinst-x86_64-23.iso \
 -object 
tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,endpoint=server

  $ qmp-shell /tmp/qmp
  (qmp) nbd-server-start addr={"host":"localhost","port":"9000"} tls-creds=tls0
  (qmp) nbd-server-add device=ide0-hd0

The first 6 patches are the conversion to the I/O channels
framework.

The next 5 patches are general tweaks to QEMU's impl of the
NBD protocol for better compliance and/or future proofing.

The next patch provides the NBD protocol TLS implementation.

The final 3 patches allow TLS to be enabled in the QEMU NBD
client and servers.


Daniel P. Berrange (15):
  qom: add user_creatable_add & user_creatable_del methods
  qemu-nbd: add support for --object command line arg
  nbd: convert block client to use I/O channels for connection setup
  nbd: convert qemu-nbd server to use I/O channels for connection setup
  nbd: convert blockdev NBD server to use I/O channels for connection
setup
  nbd: convert to using I/O channels for actual socket I/O
  nbd: invert client logic for negotiating protocol version
  nbd: make server compliant with fixed newstyle spec
  nbd: make client request fixed new style if advertized
  nbd: allow setting of an export name for qemu-nbd server
  nbd: pick first exported volume if no export name is requested
  nbd: implement TLS support in the protocol negotiation
  nbd: enable use of TLS with NBD block driver
  nbd: enable use of TLS with qemu-nbd server
  nbd: enable use of TLS with nbd-server-start command

 Makefile   

[Qemu-block] [PATCH 05/15] nbd: convert blockdev NBD server to use I/O channels for connection setup

[Daniel P. Berrange]

This converts the blockdev NBD server to use the QIOChannelSocket
class for initial listener socket setup and accepting of client
connections. Actual I/O is still being performed against the
socket file descriptor using the POSIX socket APIs.

Signed-off-by: Daniel P. Berrange 
---
 blockdev-nbd.c | 53 -
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index bcdd18b..71ee021 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -17,33 +17,49 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "block/nbd.h"
-#include "qemu/sockets.h"
+#include "io/channel-socket.h"
 
-static int server_fd = -1;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
 
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
+   gpointer opaque)
 {
-struct sockaddr_in addr;
-socklen_t addr_len = sizeof(addr);
+QIOChannelSocket *cioc;
+int fd;
 
-int fd = accept(server_fd, (struct sockaddr *), _len);
-if (fd >= 0 && !nbd_client_new(NULL, fd, nbd_client_put)) {
-shutdown(fd, 2);
+cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+ NULL);
+if (!cioc) {
+return TRUE;
+}
+
+fd = dup(cioc->fd);
+if (fd >= 0 &&
+!nbd_client_new(NULL, fd, nbd_client_put)) {
 close(fd);
 }
+object_unref(OBJECT(cioc));
+return TRUE;
 }
 
 void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
 {
-if (server_fd != -1) {
+if (server_ioc) {
 error_setg(errp, "NBD server already running");
 return;
 }
 
-server_fd = socket_listen(addr, errp);
-if (server_fd != -1) {
-qemu_set_fd_handler(server_fd, nbd_accept, NULL, NULL);
+server_ioc = qio_channel_socket_new();
+if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+return;
 }
+
+server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+ G_IO_IN,
+ nbd_accept,
+ NULL,
+ NULL);
 }
 
 /*
@@ -78,7 +94,7 @@ void qmp_nbd_server_add(const char *device, bool 
has_writable, bool writable,
 NBDExport *exp;
 NBDCloseNotifier *n;
 
-if (server_fd == -1) {
+if (!server_ioc) {
 error_setg(errp, "NBD server not running");
 return;
 }
@@ -128,9 +144,12 @@ void qmp_nbd_server_stop(Error **errp)
 nbd_close_notifier(>n, nbd_export_get_blockdev(cn->exp));
 }
 
-if (server_fd != -1) {
-qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
-close(server_fd);
-server_fd = -1;
+if (server_watch != -1) {
+g_source_remove(server_watch);
+server_watch = -1;
+}
+if (server_ioc) {
+object_unref(OBJECT(server_ioc));
+server_ioc = NULL;
 }
 }
-- 
2.5.0

[Qemu-block] [PATCH 01/15] qom: add user_creatable_add & user_creatable_del methods

[Daniel P. Berrange]

The QMP monitor code has two helper methods object_add
and qmp_object_del that are called from several places
in the code (QMP, HMP and main emulator startup).

We soon need to use this code from qemu-img, qemu-io
and qemu-nbd too, but don't want those to depend on
the monitor.

To avoid this, move object_add to user_creatable_add
an qmp_object_del to user_creatable_del, in the
object_interfaces.c file

Signed-off-by: Daniel P. Berrange 
---
 hmp.c   | 11 --
 include/monitor/monitor.h   |  3 --
 include/qom/object_interfaces.h | 31 +
 qmp.c   | 75 
 qom/object_interfaces.c | 76 +
 vl.c|  8 +++--
 6 files changed, 127 insertions(+), 77 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2140605..6044db3 100644
--- a/hmp.c
+++ b/hmp.c
@@ -29,6 +29,7 @@
 #include "qapi/string-output-visitor.h"
 #include "qapi/util.h"
 #include "qapi-visit.h"
+#include "qom/object_interfaces.h"
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
@@ -1662,6 +1663,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 void *dummy = NULL;
 OptsVisitor *ov;
 QDict *pdict;
+Object *obj = NULL;
 
 opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, );
 if (err) {
@@ -1688,12 +1690,12 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 goto out_end;
 }
 
-object_add(type, id, pdict, opts_get_visitor(ov), );
+obj = user_creatable_add(type, id, pdict, opts_get_visitor(ov), );
 
 out_end:
 visit_end_struct(opts_get_visitor(ov), _end);
 if (!err && err_end) {
-qmp_object_del(id, NULL);
+user_creatable_del(id, NULL);
 }
 error_propagate(, err_end);
 out_clean:
@@ -1704,6 +1706,9 @@ out_clean:
 g_free(id);
 g_free(type);
 g_free(dummy);
+if (obj) {
+object_unref(obj);
+}
 
 out:
 hmp_handle_error(mon, );
@@ -1936,7 +1941,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
 const char *id = qdict_get_str(qdict, "id");
 Error *err = NULL;
 
-qmp_object_del(id, );
+user_creatable_del(id, );
 hmp_handle_error(mon, );
 }
 
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 91b95ae..aa0f373 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -43,9 +43,6 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
   void *opaque);
 
-void object_add(const char *type, const char *id, const QDict *qdict,
-Visitor *v, Error **errp);
-
 AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
 bool has_opaque, const char *opaque,
 Error **errp);
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 283ae0d..3e2afeb 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -2,6 +2,8 @@
 #define OBJECT_INTERFACES_H
 
 #include "qom/object.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/visitor.h"
 
 #define TYPE_USER_CREATABLE "user-creatable"
 
@@ -72,4 +74,33 @@ void user_creatable_complete(Object *obj, Error **errp);
  * from implements USER_CREATABLE interface.
  */
 bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
+
+/**
+ * user_creatable_add:
+ * @type: the object type name
+ * @id: the unique ID for the object
+ * @qdict: the object parameters
+ * @v: the visitor
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object @type, placing
+ * it in the object composition tree with name @id, initializing
+ * it with properties from @qdict
+ *
+ * Returns: the newly created object or NULL on error
+ */
+Object *user_creatable_add(const char *type, const char *id,
+   const QDict *qdict,
+   Visitor *v, Error **errp);
+
+/**
+ * user_creatable_del:
+ * @id: the unique ID for the object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Delete an instance of the user creatable object identified
+ * by @id.
+ */
+void user_creatable_del(const char *id, Error **errp);
+
 #endif
diff --git a/qmp.c b/qmp.c
index 0a1fa19..70fdecd 100644
--- a/qmp.c
+++ b/qmp.c
@@ -622,65 +622,13 @@ void qmp_add_client(const char *protocol, const char 
*fdname,
 close(fd);
 }
 
-void object_add(const char *type, const char *id, const QDict *qdict,
-Visitor *v, Error **errp)
-{
-Object *obj;
-ObjectClass *klass;
-const QDictEntry *e;
-Error *local_err = NULL;
-
-klass = object_class_by_name(type);
-if (!klass) {
-error_setg(errp, "invalid object type: %s", type);
-return;
-}
-
-if 

[Qemu-block] [PATCH 07/15] nbd: invert client logic for negotiating protocol version

[Daniel P. Berrange]

The nbd_receive_negotiate() method takes different code
paths based on whether 'name == NULL', and then checks
the expected protocol version in each branch.

This patch inverts the logic, so that it takes different
code paths based on what protocol version it receives and
then checks if name is NULL or not as needed.

This facilitates later code which allows the client to
be caapble of using the new style protocol regardless
of whether an export name is listed or not.

Signed-off-by: Daniel P. Berrange 
---
 nbd.c | 60 +---
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/nbd.c b/nbd.c
index 701b951..bdfc45e 100644
--- a/nbd.c
+++ b/nbd.c
@@ -682,20 +682,11 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
 magic = be64_to_cpu(magic);
 TRACE("Magic is 0x%" PRIx64, magic);
 
-if (name) {
+if (magic == NBD_OPTS_MAGIC) {
 uint32_t reserved = 0;
 uint32_t opt;
 uint32_t namesize;
 
-TRACE("Checking magic (opts_magic)");
-if (magic != NBD_OPTS_MAGIC) {
-if (magic == NBD_CLIENT_MAGIC) {
-error_setg(errp, "Server does not support export names");
-} else {
-error_setg(errp, "Bad magic received");
-}
-goto fail;
-}
 if (read_sync(ioc, , sizeof(tmp)) != sizeof(tmp)) {
 error_setg(errp, "Failed to read server flags");
 goto fail;
@@ -708,6 +699,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
 goto fail;
 }
 /* write the export name */
+if (!name) {
+error_setg(errp, "Server requires an export name");
+goto fail;
+}
 magic = cpu_to_be64(magic);
 if (write_sync(ioc, , sizeof(magic)) != sizeof(magic)) {
 error_setg(errp, "Failed to send export name magic");
@@ -728,39 +723,42 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
 error_setg(errp, "Failed to send export name");
 goto fail;
 }
-} else {
-TRACE("Checking magic (cli_magic)");
 
-if (magic != NBD_CLIENT_MAGIC) {
-if (magic == NBD_OPTS_MAGIC) {
-error_setg(errp, "Server requires an export name");
-} else {
-error_setg(errp, "Bad magic received");
-}
+if (read_sync(ioc, , sizeof(s)) != sizeof(s)) {
+error_setg(errp, "Failed to read export length");
 goto fail;
 }
-}
+*size = be64_to_cpu(s);
+TRACE("Size is %" PRIu64, *size);
 
-if (read_sync(ioc, , sizeof(s)) != sizeof(s)) {
-error_setg(errp, "Failed to read export length");
-goto fail;
-}
-*size = be64_to_cpu(s);
-TRACE("Size is %" PRIu64, *size);
+if (read_sync(ioc, , sizeof(tmp)) != sizeof(tmp)) {
+error_setg(errp, "Failed to read export flags");
+goto fail;
+}
+*flags |= be16_to_cpu(tmp);
+} else if (magic == NBD_CLIENT_MAGIC) {
+if (name) {
+error_setg(errp, "Server does not support export names");
+goto fail;
+}
+
+if (read_sync(ioc, , sizeof(s)) != sizeof(s)) {
+error_setg(errp, "Failed to read export length");
+goto fail;
+}
+*size = be64_to_cpu(s);
+TRACE("Size is %" PRIu64, *size);
 
-if (!name) {
 if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
 error_setg(errp, "Failed to read export flags");
 goto fail;
 }
 *flags = be32_to_cpup(flags);
 } else {
-if (read_sync(ioc, , sizeof(tmp)) != sizeof(tmp)) {
-error_setg(errp, "Failed to read export flags");
-goto fail;
-}
-*flags |= be16_to_cpu(tmp);
+error_setg(errp, "Bad magic received");
+goto fail;
 }
+
 if (read_sync(ioc, , 124) != 124) {
 error_setg(errp, "Failed to read reserved block");
 goto fail;
-- 
2.5.0

[Qemu-block] [PATCH 03/15] nbd: convert block client to use I/O channels for connection setup

[Daniel P. Berrange]

This converts the NBD block driver client to use the QIOChannelSocket
class for initial connection setup. The NbdClientSession struct has
two pointers, one to the master QIOChannelSocket providing the raw
data channel, and one to a QIOChannel which is the current channel
used for I/O. Initially the two point to the same object, but when
TLS support is added, they will point to different objects.

In this initial conversion though, all I/O is still actually done
using the raw POSIX sockets APIs.

Signed-off-by: Daniel P. Berrange 
---
 Makefile   |  6 +++---
 block/nbd-client.c | 59 --
 block/nbd-client.h |  8 ++--
 block/nbd.c| 39 ++--
 tests/Makefile |  2 +-
 5 files changed, 61 insertions(+), 53 deletions(-)

diff --git a/Makefile b/Makefile
index b0049ea..840e99c 100644
--- a/Makefile
+++ b/Makefile
@@ -234,9 +234,9 @@ util/module.o-cflags = 
-D'CONFIG_BLOCK_MODULES=$(block-modules)'
 
 qemu-img.o: qemu-img-cmds.h
 
-qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) 
libqemuutil.a libqemustub.a
-qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) 
libqemuutil.a libqemustub.a
-qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) 
libqemuutil.a libqemustub.a
+qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) 
$(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) 
$(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) 
$(qom-obj-y) libqemuutil.a libqemustub.a
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index b7fd17a..bf5b0a3 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -27,7 +27,6 @@
  */
 
 #include "nbd-client.h"
-#include "qemu/sockets.h"
 
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ ((uint64_t)(intptr_t)bs))
@@ -48,12 +47,16 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 NbdClientSession *client = nbd_get_client_session(bs);
 
 /* finish any pending coroutines */
-shutdown(client->sock, 2);
+qio_channel_shutdown(client->ioc,
+ QIO_CHANNEL_SHUTDOWN_BOTH,
+ NULL);
 nbd_recv_coroutines_enter_all(client);
 
 nbd_client_detach_aio_context(bs);
-closesocket(client->sock);
-client->sock = -1;
+object_unref(OBJECT(client->sioc));
+client->sioc = NULL;
+object_unref(OBJECT(client->ioc));
+client->ioc = NULL;
 }
 
 static void nbd_reply_ready(void *opaque)
@@ -68,7 +71,7 @@ static void nbd_reply_ready(void *opaque)
  * that another thread has done the same thing in parallel, so
  * the socket is not readable anymore.
  */
-ret = nbd_receive_reply(s->sock, >reply);
+ret = nbd_receive_reply(s->sioc->fd, >reply);
 if (ret == -EAGAIN) {
 return;
 }
@@ -124,27 +127,23 @@ static int nbd_co_send_request(BlockDriverState *bs,
 s->send_coroutine = qemu_coroutine_self();
 aio_context = bdrv_get_aio_context(bs);
 
-aio_set_fd_handler(aio_context, s->sock, false,
+aio_set_fd_handler(aio_context, s->sioc->fd, false,
nbd_reply_ready, nbd_restart_write, bs);
 if (qiov) {
-if (!s->is_unix) {
-socket_set_cork(s->sock, 1);
-}
-rc = nbd_send_request(s->sock, request);
+qio_channel_set_cork(s->ioc, true);
+rc = nbd_send_request(s->sioc->fd, request);
 if (rc >= 0) {
-ret = qemu_co_sendv(s->sock, qiov->iov, qiov->niov,
+ret = qemu_co_sendv(s->sioc->fd, qiov->iov, qiov->niov,
 offset, request->len);
 if (ret != request->len) {
 rc = -EIO;
 }
 }
-if (!s->is_unix) {
-socket_set_cork(s->sock, 0);
-}
+qio_channel_set_cork(s->ioc, false);
 } else {
-rc = nbd_send_request(s->sock, request);
+rc = nbd_send_request(s->sioc->fd, request);
 }
-aio_set_fd_handler(aio_context, s->sock, false,
+aio_set_fd_handler(aio_context, s->sioc->fd, false,
nbd_reply_ready, NULL, bs);
 s->send_coroutine = NULL;
 qemu_co_mutex_unlock(>send_mutex);
@@ -165,7 +164,7 @@ static void nbd_co_receive_reply(NbdClientSession *s,
 reply->error = EIO;
 } else {
 if (qiov && reply->error == 0) {
-ret = qemu_co_recvv(s->sock, qiov->iov, qiov->niov,
+ret = qemu_co_recvv(s->sioc->fd, qiov->iov, qiov->niov,
 offset, request->len);
 if (ret != request->len) {
 reply->error = EIO;
@@ -349,14 +348,14 @@ int 

[Qemu-block] [PATCH 04/15] nbd: convert qemu-nbd server to use I/O channels for connection setup

[Daniel P. Berrange]

This converts the qemu-nbd server to use the QIOChannelSocket
class for initial listener socket setup and accepting of client
connections. Actual I/O is still being performed against the
socket file descriptor using the POSIX socket APIs.

In this initial conversion though, all I/O is still actually done
using the raw POSIX sockets APIs.

Signed-off-by: Daniel P. Berrange 
---
 qemu-nbd.c | 91 ++
 1 file changed, 50 insertions(+), 41 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 41f4285..28ab54f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -21,7 +21,6 @@
 #include "block/block_int.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
-#include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "block/snapshot.h"
@@ -29,16 +28,13 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi/opts-visitor.h"
 #include "qom/object_interfaces.h"
+#include "io/channel-socket.h"
 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
@@ -58,7 +54,8 @@ static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
 static int shared = 1;
 static int nb_fds;
-static int server_fd;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
 
 static void usage(const char *name)
 {
@@ -241,19 +238,21 @@ static void *nbd_client_thread(void *arg)
 char *device = arg;
 off_t size;
 uint32_t nbdflags;
-int fd, sock;
+QIOChannelSocket *sioc;
+int fd;
 int ret;
 pthread_t show_parts_thread;
 Error *local_error = NULL;
 
-
-sock = socket_connect(saddr, _error, NULL, NULL);
-if (sock < 0) {
+sioc = qio_channel_socket_new();
+if (qio_channel_socket_connect_sync(sioc,
+saddr,
+_error) < 0) {
 error_report_err(local_error);
 goto out;
 }
 
-ret = nbd_receive_negotiate(sock, NULL, ,
+ret = nbd_receive_negotiate(sioc->fd, NULL, ,
 , _error);
 if (ret < 0) {
 if (local_error) {
@@ -270,7 +269,7 @@ static void *nbd_client_thread(void *arg)
 goto out_socket;
 }
 
-ret = nbd_init(fd, sock, nbdflags, size);
+ret = nbd_init(fd, sioc->fd, nbdflags, size);
 if (ret < 0) {
 goto out_fd;
 }
@@ -291,13 +290,14 @@ static void *nbd_client_thread(void *arg)
 goto out_fd;
 }
 close(fd);
+object_unref(OBJECT(sioc));
 kill(getpid(), SIGTERM);
 return (void *) EXIT_SUCCESS;
 
 out_fd:
 close(fd);
 out_socket:
-closesocket(sock);
+object_unref(OBJECT(sioc));
 out:
 kill(getpid(), SIGTERM);
 return (void *) EXIT_FAILURE;
@@ -314,7 +314,7 @@ static void nbd_export_closed(NBDExport *exp)
 state = TERMINATED;
 }
 
-static void nbd_update_server_fd_handler(int fd);
+static void nbd_update_server_watch(void);
 
 static void nbd_client_closed(NBDClient *client)
 {
@@ -322,41 +322,51 @@ static void nbd_client_closed(NBDClient *client)
 if (nb_fds == 0 && !persistent && state == RUNNING) {
 state = TERMINATE;
 }
-nbd_update_server_fd_handler(server_fd);
+nbd_update_server_watch();
 nbd_client_put(client);
 }
 
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 {
-struct sockaddr_in addr;
-socklen_t addr_len = sizeof(addr);
+QIOChannelSocket *cioc;
+int fd;
 
-int fd = accept(server_fd, (struct sockaddr *), _len);
-if (fd < 0) {
-perror("accept");
-return;
+cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+ NULL);
+if (!cioc) {
+return TRUE;
 }
 
 if (state >= TERMINATE) {
-close(fd);
-return;
+object_unref(OBJECT(cioc));
+return TRUE;
 }
 
-if (nbd_client_new(exp, fd, nbd_client_closed)) {
+fd = dup(cioc->fd);
+if (fd >= 0 &&
+nbd_client_new(exp, fd, nbd_client_closed)) {
 nb_fds++;
-nbd_update_server_fd_handler(server_fd);
-} else {
-shutdown(fd, 2);
-close(fd);
+nbd_update_server_watch();
 }
+object_unref(OBJECT(cioc));
+
+return TRUE;
 }
 
-static void nbd_update_server_fd_handler(int fd)
+static void nbd_update_server_watch(void)
 {
 if (nbd_can_accept()) {
-qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
+if (server_watch == -1) {
+server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+ G_IO_IN,
+ nbd_accept,
+ NULL, NULL);
+}
 } else {
-qemu_set_fd_handler(fd, NULL, NULL, NULL);
+if 

Re: [Qemu-block] [PATCH 00/15] Implement TLS support to QEMU NBD server & client

[Wouter Verhelst]

[nbd-general added to Cc]

Hi Daniel,

On Fri, Nov 27, 2015 at 12:20:38PM +, Daniel P. Berrange wrote:
> This series of patches implements support for TLS in the QEMU NBD
> server and client code.
> 
> It is implementing the NBD_OPT_STARTTLS option that was previously
> discussed here:
> 
>   https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html
> 
> And is also described in the NBD spec here:
> 
>   https://github.com/yoe/nbd/blob/master/doc/proto.md
> 
> Based on my impl I think the NBD_OPT_STARTTLS specification is
> fine. In my impl, when TLS is requested in the server, I chose
> to *refuse* all NBD options until NBD_OPT_STARTTLS has completed
> successfully. There is an annoying thing about the fixed new style
> protocol in that it made an exception for NBD_OPT_EXPORT_NAME,
> so that it is still not possible to report errors for that option.
> 
> [qupte]
>  - The server will reply to any option apart from `NBD_OPT_EXPORT_NAME`
> with reply packets in the following format:
> 
>   S: 64 bits, `0x3e889045565a9` (magic number for replies)  
>   S: 32 bits, the option as sent by the client to which this is a reply  
>   S: 32 bits, reply type (e.g., `NBD_REP_ACK` for successful completion,
>  or `NBD_REP_ERR_UNSUP` to mark use of an option not known by this
>  server  
>   S: 32 bits, length of the reply. This may be zero for some replies, in
>  which case the next field is not sent  
>   S: any data as required by the reply (e.g., an export name in the case
>  of `NBD_REP_SERVER`)
> [/quote]
> 
> I wish those words "apart from NBD_OPT_EXPORT_NAME" were not there
> for fixed new style protocol - maybe it needs a very-fixed new
> style protocol...

Yes. This was done in this way to more easily retain backwards compatibility
with non-fixed newstyle. In hindsight, it was probably a mistake to do
things that way, but it's not going to be easy to turn back the clock
now...

I have been thinking of adding a message NBD_OPT_SELECT_EXPORT to
replace NBD_OPT_EXPORT_NAME, which would select an export but not end
negotiation. That would also require another message to end negotiation
and move on to the transmission phase, obviously.

The negotation would then look something like:

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_GO
 <- NBD_REP_ACK
[transmission phase]

or, to add some error conditions:

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ERR_UNSUP # this server doesn't support SELECT_EXPORT
NBD_OPT_EXPORT_NAME # fall back to older message
[transmission phase]

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ERR_TLS_REQD # this export requires TLS
NBD_OPT_STARTTLS
 <- NBD_REP_ACK
[... tls ...]
NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_GO
 <- NBD_REP_ACK
[transmission phase]

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_STARTTLS
 <- NBD_REP_ACK
[... tls ...]
NBD_OPT_GO
 <- NBD_REP_ERR_INVALID # export selected before starttls is not retained

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_SELECT_EXPORT (we actually want this other one)
 <- NBD_REP_ERR_UNKNOWN # unknown export
NBD_OPT_GO
 <- NBD_REP_ERR_INVALID # second select cleared the result of the first one

etc

I'm not sure whether it's worth the trouble, though.

> As is, if the client connects to a TLS enabled NBD server and then
> immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us
> to send back NBD_REP_ERR_TLS_REQD as the spec requires that the
> server close the connection :-(  For this reason I have made the
> qemu NBD client always send NBD_OPT_LIST as the first thing it
> does, so that we can see the NBD_REP_ERR_TLS_REQD response.

I suppose that is a valid workaround.

> I chose to *NOT* implement the NBD_OPT_PEEK_EXPORT option as it is
> inherently insecure to have a client to ask the server which
> exports need TLS, while the protocol is still running in plain text
> mode, as it allows a trivial MITM downgrade attack. I can see value
> in the NBD_OPT_PEEK_EXPORT option for probing other features, but
> only after TLS has been negotiated. As such I believe NBD_F_EXP_TLS_OK
> and NBD_F_EXP_TLS_REQ should be deleted from the spec as it is not
> possible to use them in a secure manner.

Mm. Fair enough.

The reason for adding that flag was to support encrypted and
non-encrypted exports from the same server, but I suppose we can signal
those things with NBD_REP_ERR_TLS_REQD and NBD_REP_ERR_POLICY, too.

> This series of patches has my v3 I/O channels patch series as a
> pre-requisite:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04208.html
> 
> Usage of TLS is described in the commit messages for each patch,
> but for sake of people who don't want to explore the series, here's
> the summary
> 
> Starting QEMU system emulator with a disk backed by an TLS encrypted
> NBD export
> 
>   $ qemu-system-x86_64 \
>  -object 
> tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls
>  -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0
> 
> Starting a standalone NBD server 

Re: [Qemu-block] [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Nov 25, 2015, at 11:29 PM, Eric Blake wrote:

> On 11/25/2015 09:23 PM, Eric Blake wrote:
> 
>>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t 
>>> *mediaIterator,
>>> +char 
>>> *mediaType)
>> 
>> Unusual indentation; more typical is:
>> 
>> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
>> *mediaIterator,
>> | char *mediatType)
> 
> And then my mailer messes it up :(
> 
>> static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>>   char *mediatType)
> 
> Let's see if that's better (the 'char' is directly beneath the
> 'io_iterator_t').

In my email program, the 'char' appears underneath the Ejectable word. When I 
change the font to monaco (A mono-spaced font), the 'char' does appear 
underneath the io_iterator_t. 

Re: [Qemu-block] [PATCH v2 20/21] qemu-iotests: Test cache mode option inheritance

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> This is doing a more complete test on setting cache modes both while
> opening an image (i.e. in a -drive command line) and in reopen
> situations. It checks that reopen can specify options for child nodes
> and that cache modes are correctly inherited from parent nodes where
> they are not specified.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/142 | 354 
>  tests/qemu-iotests/142.out | 788 
> +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 1143 insertions(+)
>  create mode 100755 tests/qemu-iotests/142
>  create mode 100644 tests/qemu-iotests/142.out

Comments below, to make it short: With %s/bs->backing_hd/bs->backing/
and a comment about why you commented 'out | grep "Cache"':

Reviewed-by: Max Reitz 

> diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
> new file mode 100755
> index 000..58daf26
> --- /dev/null
> +++ b/tests/qemu-iotests/142
> @@ -0,0 +1,354 @@
> +#!/bin/bash
> +#
> +# Test for configuring cache modes of arbitrary nodes (requires O_DIRECT)
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +# creator
> +owner=kw...@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +
> +_cleanup()
> +{
> +_cleanup_test_img
> +rm -f $TEST_IMG.snap
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +# We test all cache modes anyway, but O_DIRECT needs to be supported
> +_default_cache_mode none
> +_supported_cache_modes none directsync
> +
> +function do_run_qemu()
> +{
> +echo Testing: "$@"
> +(
> +if ! test -t 0; then
> +while read cmd; do
> +echo $cmd
> +done
> +fi
> +echo quit
> +) | $QEMU -nographic -monitor stdio -nodefaults "$@"
> +echo
> +}
> +
> +function run_qemu()
> +{
> +do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu
> +}
> +
> +size=128M
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $size
> +TEST_IMG="$TEST_IMG.snap" _make_test_img $size
> +_make_test_img -b "$TEST_IMG.base" $size
> +
> +echo
> +echo === Simple test for all cache modes ===
> +echo
> +
> +run_qemu -drive file="$TEST_IMG",cache=none
> +run_qemu -drive file="$TEST_IMG",cache=directsync
> +run_qemu -drive file="$TEST_IMG",cache=writeback
> +run_qemu -drive file="$TEST_IMG",cache=writethrough
> +run_qemu -drive file="$TEST_IMG",cache=unsafe
> +run_qemu -drive file="$TEST_IMG",cache=invalid_value
> +
> +echo
> +echo === Check inheritance of cache modes ===
> +echo
> +
> +files="if=none,file=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
> +ids="node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file"
> +
> +function check_cache_all()
> +{
> +# cache.direct is supposed to be inherited by both bs->file and
> +# bs->backing_hd

%s/bs->backing_hd/bs->backing/g

> +
> +echo -e "cache.direct=on on none0"
> +echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.direct=on | 
> grep "Cache"
> +echo -e "\ncache.direct=on on file"
> +echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.direct=on 
> | grep "Cache"
> +echo -e "\ncache.direct=on on backing"
> +echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.cache.direct=on | grep "Cache"
> +echo -e "\ncache.direct=on on backing-file"
> +echo "$hmp_cmds" | run_qemu -drive 
> "$files","$ids",backing.file.cache.direct=on | grep "Cache"
> +
> +# cache.writeback is supposed to be inherited by bs->backing_hd; bs->file
> +# always gets cache.writeback=on

I don't know whether having the backing file inherit cache.writeback
makes any sense, but I guess it's the default behavior and overriding it
wouldn't make any sense either.

> +echo -e "\n\ncache.writeback=off on none0"
> +echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.writeback=off | 
> grep "Cache"
> +echo -e "\ncache.writeback=off on file"
> +echo "$hmp_cmds" | run_qemu -drive 
>