Re: [Qemu-block] [PATCH] nvme: do not advertise support for unsupported arbitration mechanism

[Max Reitz]

On 06.06.19 11:25, Klaus Birkelund Jensen wrote:
> The device mistakenly reports that the Weighted Round Robin with Urgent
> Priority Class arbitration mechanism is supported.
> 
> It is not.

I believe you based on the fact that there is no “weight” or “priority”
anywhere in nvme.c, and that it does not evaluate the Arbitration
Mechanism Selected field.

> Signed-off-by: Klaus Birkelund Jensen 
> ---
>  hw/block/nvme.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 30e50f7a3853..415b4641d6b4 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1383,7 +1383,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> **errp)
>  n->bar.cap = 0;
>  NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
>  NVME_CAP_SET_CQR(n->bar.cap, 1);
> -NVME_CAP_SET_AMS(n->bar.cap, 1);

I suppose the better way would be to pass 0, so it is more explicit, I
think.

(Just removing it looks like it may have just been forgotten.)

Max

>  NVME_CAP_SET_TO(n->bar.cap, 0xf);
>  NVME_CAP_SET_CSS(n->bar.cap, 1);
>  NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 21/42] block: Use CAFs for debug breakpoints

[Eric Blake]

On 6/14/19 11:12 AM, Max Reitz wrote:
> On 14.06.19 17:29, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> When looking for a blkdebug node (which implements debug breakpoints),
>>> use bdrv_primary_bs() to iterate through the graph, because that is
>>> where a blkdebug node would be.
>>>
>>> Signed-off-by: Max Reitz 
>>
>> Honestly, don't know why blkdebug is always searched in ->file sequence,
> 
> Usually, blkdebug is just above the protocol node.  So
> 
> $format --file--> $protocol
> 
> becomes
> 
> $format --file--> blkdebug --file--> $protocol
> 
> This is why the existing code generally looks for blkdebug under the
> ->file link.

blkdebug is an interesting beast; there are use cases for both:

blkdebug -> qcow2 -> file

for debugging only guest-visible actions, and

qcow2 -> blkdebug -> file

for debugging specific qcow2 metadata actions.


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver

[Max Reitz]

On 14.06.19 18:22, Vladimir Sementsov-Ogievskiy wrote:
> 14.06.2019 15:57, Max Reitz wrote:
>> On 14.06.19 11:04, Vladimir Sementsov-Ogievskiy wrote:
>>> 13.06.2019 18:57, Max Reitz wrote:
 On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> Backup-top filter does copy-before-write operation. It should be
> inserted above active disk and has a target node for CBW, like the
> following:
>
>   +---+
>   | Guest |
>   +---+
>   |r,w
>   v
>   ++  target   +---+
>   | backup_top |-->| target(qcow2) |
>   ++   CBW +---+
>   |
> backing |r,w
>   v
>   +-+
>   | Active disk |
>   +-+
>
> The driver will be used in backup instead of write-notifiers.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>block/backup-top.h  |  64 +
>block/backup-top.c  | 322 
>block/Makefile.objs |   2 +
>3 files changed, 388 insertions(+)
>create mode 100644 block/backup-top.h
>create mode 100644 block/backup-top.c
>
> diff --git a/block/backup-top.h b/block/backup-top.h
> new file mode 100644
> index 00..788e18c358
> --- /dev/null
> +++ b/block/backup-top.h
>>
>> [...]
>>
> +/*
> + * bdrv_backup_top_append
> + *
> + * Append backup_top filter node above @source node. @target node will 
> receive
> + * the data backed up during CBE operations. New filter together with 
> @target
> + * node are attached to @source aio context.
> + *
> + * The resulting filter node is implicit.

 Why?  It’s just as easy for the caller to just make it implicit if it
 should be.  (And usually the caller should decide that.)
>>>
>>> Personally, I don't know what are the reasons for filters to bi implicit or 
>>> not,
>>> I just made it like other job-filters.. I can move making-implicit to the 
>>> caller
>>> or drop it at all (if it will work).
>>
>> Nodes are implicit if they haven’t been added consciously by the user.
>> A node added by a block job can be non-implicit, too, as mirror shows;
>> If the user specifies the filter-node-name option, they will know about
>> the node, thus it is no longer implicit.
>>
>> If the user doesn’t know about the node (they didn’t give the
>> filter-node-name option), the node is implicit.
>>
> 
> Ok, I understand it. But it doesn't show, why it should be implicit?
> Isn't it less buggy to make all filters explicit? We don't hide implicit nodes
> from query-named-block-nodes (the only interface to explore the whole graph 
> for now)
> anyway. And we can't absolutely hide side effects of additional node in the 
> graph.

Well, we try, at least.  At least we hide them from query-block.

> So, is there any real benefit of supporting separation into implicit and 
> explicit filters?
> It seems for me that it only complicates things...
> In other words, what will break if we make all filters explicit?

The theory is that qemu may decide to add nodes at any point, but at
least when managing chains etc., they may not be visible to the user.  I
don’t think we can get rid of them so easily.

One example that isn’t implemented yet is copy-on-read.  In theory,
specifying copy-on-read=on for -drive should create an implicit COR node
on top.  The user shouldn’t see that node when inspecting the drive or
when issuing block jobs on it, etc.  And this node has to stay there
when the user does e.g. an active commit somewhere down the chain.

That sounds like a horrible ordeal to implement, so it hasn’t been done
yet.  Maybe it never will.  It isn’t that bad for the job filters,
because they generally freeze the block graph, so there is no problem
with potential modifications.

All in all I do think having implicit nodes makes sense.  Maybe not so
much now, but in the future (if someone implements converting -drive COR
and throttle options to implicit nodes...).

>> [...]
>>
> +static int coroutine_fn backup_top_co_flush(BlockDriverState *bs)
> +{
> +if (!bs->backing) {
> +return 0;
> +}
> +
> +return bdrv_co_flush(bs->backing->bs);

 Should we flush the target, too?
>>>
>>> Hm, you've asked it already, on previous version :)
>>
>> I wasn’t sure...
>>
>>> Backup don't do it,
>>> so I just keep old behavior. And what is the reason to flush backup target
>>> on any guest flush?
>>
>> Hm, well, what’s the reason not to do it?
> 
> guest flushes will be slowed down?

Hm, the user could specify cache=unsafe if they don’t care.  Which gives
me second thoughs... [1]

>> Also, there are not only guest flushes.  bdrv_flush_all() exists, which
>> is called when the guest is stopped.  So who is going to flush the
>> target if not its parent?
>>
>> 

Re: [Qemu-block] [PATCH v5 23/42] blockdev: Use CAF in external_snapshot_prepare()

[Vladimir Sementsov-Ogievskiy]

14.06.2019 19:20, Max Reitz wrote:
> On 14.06.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> This allows us to differentiate between filters and nodes with COW
>>> backing files: Filters cannot be used as overlays at all (for this
>>> function).
>>>
>>> Signed-off-by: Max Reitz 
>>
>> Overlay created in snapshot operation assumed to consume following writes
>> and it's filtered child becomes readonly.. And filter works in completely 
>> another
>> way.
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>>
>> [hmm, I start to like using "filtered child" collocation when I say about 
>> this thing.
>>didn't you think about renaming backing chain to filtered chain?]
> 
> Hm.  There are backing chains and there are backing chains.  There are
> qemu-internal backing chains that consist of a healthy mix of filters
> and COW overlays, and then there are the more high-level backing chains
> the user actually manages, where only the overlays are important.
> 
> I think it would make sense to rename the “qemu-internal backing chains"
> to “filter chains” or something.  But that makes it sound a bit like it
> would only mean R/W filters...  Maybe just “chain”?
> 
> Actually, the only functions I find are is_backing_chain_frozen & Co,
> and they could simply become is_chain_frozen.  Is there anything else?

Chain is too general, may be, blockchain? :)))

And to be serious, one more reason to rename it is yours
bdrv_backing_chain_next which is about user-backing-chain and differs from
frozen-chain related functions.

However, I don't think that these series is good place for this renaming,
it's rather big already.

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Vladimir Sementsov-Ogievskiy]

14.06.2019 19:02, Max Reitz wrote:
> On 14.06.19 16:31, Vladimir Sementsov-Ogievskiy wrote:
>> 14.06.2019 16:50, Max Reitz wrote:
>>> On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
 13.06.2019 1:09, Max Reitz wrote:
> Use child access functions when iterating through backing chains so
> filters do not break the chain.
>
> Signed-off-by: Max Reitz 
> ---
> block.c | 40 
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/block.c b/block.c
> index 11f37983d9..505b3e9a01 100644
> --- a/block.c
> +++ b/block.c
>>>
>>> [...]
>>>
> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
> BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
> BlockDriverState *bs)
> {
> -while (active && bs != backing_bs(active)) {
> -active = backing_bs(active);
> +bs = bdrv_skip_rw_filters(bs);
> +active = bdrv_skip_rw_filters(active);
> +
> +while (active) {
> +BlockDriverState *next = bdrv_backing_chain_next(active);
> +if (bs == next) {
> +return active;
> +}
> +active = next;
> }
> 
> -return active;
> +return NULL;
> }

 Semantics changed for this function.
 It is used in two places
 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we 
 will never have
   filter node as a bottom of some valid chain

 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't 
 understand,
 why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs 
 overlay is out of the job,
 what is this check for?
>>>
>>> There is a loop before this check which checks that the same blocker is
>>> not set on any nodes between top and base (both inclusive).  I guess
>>> non-active commit checks the node above @top, too, because its backing
>>> file will change.
>>
>> So in this case frozen chain works better.
> 
> Perhaps.  The op blockers are in this weird state anyway.  I don’t think
> we even need them any more, because the permissions were intended to
> replace them (they were originally called “fine-grained op blockers”, I
> seem to remember).
> 
> I dare not touch them.
> 
> /* Given a BDS, searches for the base layer. */
>>>
>>> [...]
>>>
> @@ -5149,7 +5165,7 @@ BlockDriverState 
> *bdrv_find_backing_image(BlockDriverState *bs,
> char *backing_file_full_ret;
> 
> if (strcmp(backing_file, curr_bs->backing_file) == 0) {

 hmm, interesting, what bs->backing_file now means? It's strange enough to 
 store such field on
 bds, when we have backing link anyway..
>>>
>>> Patch 37 has you covered. :-)
>>>
>>
>> Hmm, if it has removed this field, but it doesn't)
> 
> Because it’s needed.  (Just not in the current form, but that’s what 37
> is for.)
> 
>> So, we finished with some object, called "overlay", but it is not an overlay 
>> of bs, it's overlay of
>> first non-implicit filtered node in bs backing chain, it may be found by 
>> bdrv_find_overlay() helper (which is
>> almost unused and my be safely dropped), and filename of this "overlay" is 
>> stored in bs->backing_file string
>> variable, keeping in mind that bs->backing is pointer to backing child of bs 
>> which is completely another thing?
> 
> I don’t quite see what you mean.  There is no “overlay” in this function.

Hmm, sorry, I kept in mind overlay from the next patch..

> 
>> Oh, no, everything related to filename-based backing chain logic is not for 
>> me o_O. If something doesn't work
>> with filename-based logic users should use node-names..
> 
> In theory yes, but that isn’t an option for qemu-img commit, for example.

And if something doesn't work with qemu-img, users should use qemu process in 
stopped state. And I'd prefer to
deprecate most of qemu-img :) Actually we in Virtuozzo already go this way for 
some things. QMP interface is
a lot more useful than qemu-img, and it's always simpler to maintain and 
develop one thing than two.

> 
>> And I'd prefer to deprecate filename based interfaces at all.
> 
> Me too.
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04878.html
> 
> :-/
> 

Really sad..


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 20/42] block/snapshot: Fall back to storage child

[Vladimir Sementsov-Ogievskiy]

14.06.2019 19:10, Max Reitz wrote:
> On 14.06.19 17:22, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> If the top node's driver does not provide snapshot functionality and we
>>> want to go down the chain, we should go towards the child which stores
>>> the data, i.e. the storage child.
>>>
>>> bdrv_snapshot_goto() becomes a bit weird because we may have to redirect
>>> the actual child pointer, so it only works if the storage child is
>>> bs->file or bs->backing (and then we have to find out which it is).
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>block/snapshot.c | 74 ++--
>>>1 file changed, 53 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index f2f48f926a..58cd667f3a 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -154,8 +154,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>>>}
>>>
>>>if (!drv->bdrv_snapshot_create) {
>>> -if (bs->file != NULL) {
>>> -return bdrv_can_snapshot(bs->file->bs);
>>> +BlockDriverState *storage_bs = bdrv_storage_bs(bs);
>>> +if (storage_bs) {
>>> +return bdrv_can_snapshot(storage_bs);
>>>}
>>>return 0;
>>>}
>>
>> Hmm is it correct at all doing a snapshot, when top format node doesn't 
>> support it,
>> metadata child doesn't support it and storage child supports? Doing 
>> snapshots of
>> storage child seems useless, as data file must be in sync with metadata.
> 
> You’re right.
> 
> That’s actually a bug already.  VMDK can store data in multiple
> children, but it does not support snapshots.  So if you store such a
> split VMDK disk on an RBD volume, it is possible that just the
> descriptor file is snapshotted, but nothing else.
> 
> Hmmm.  I think the best way is to check whether there is exactly one
> child that is not the bdrv_filtered_cow_child().  If so, we can go down
> to it and snapshot it.  Otherwise, the node does not support snapshots.
> 

Anything prevents format node to store something not in a bdrv child but in 
something separate?

May be the safest way is to fall back only for filter nodes.


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy]

14.06.2019 15:57, Max Reitz wrote:
> On 14.06.19 11:04, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 18:57, Max Reitz wrote:
>>> On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
 Backup-top filter does copy-before-write operation. It should be
 inserted above active disk and has a target node for CBW, like the
 following:

   +---+
   | Guest |
   +---+
   |r,w
   v
   ++  target   +---+
   | backup_top |-->| target(qcow2) |
   ++   CBW +---+
   |
 backing |r,w
   v
   +-+
   | Active disk |
   +-+

 The driver will be used in backup instead of write-notifiers.

 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
block/backup-top.h  |  64 +
block/backup-top.c  | 322 
block/Makefile.objs |   2 +
3 files changed, 388 insertions(+)
create mode 100644 block/backup-top.h
create mode 100644 block/backup-top.c

 diff --git a/block/backup-top.h b/block/backup-top.h
 new file mode 100644
 index 00..788e18c358
 --- /dev/null
 +++ b/block/backup-top.h
> 
> [...]
> 
 +/*
 + * bdrv_backup_top_append
 + *
 + * Append backup_top filter node above @source node. @target node will 
 receive
 + * the data backed up during CBE operations. New filter together with 
 @target
 + * node are attached to @source aio context.
 + *
 + * The resulting filter node is implicit.
>>>
>>> Why?  It’s just as easy for the caller to just make it implicit if it
>>> should be.  (And usually the caller should decide that.)
>>
>> Personally, I don't know what are the reasons for filters to bi implicit or 
>> not,
>> I just made it like other job-filters.. I can move making-implicit to the 
>> caller
>> or drop it at all (if it will work).
> 
> Nodes are implicit if they haven’t been added consciously by the user.
> A node added by a block job can be non-implicit, too, as mirror shows;
> If the user specifies the filter-node-name option, they will know about
> the node, thus it is no longer implicit.
> 
> If the user doesn’t know about the node (they didn’t give the
> filter-node-name option), the node is implicit.
> 

Ok, I understand it. But it doesn't show, why it should be implicit?
Isn't it less buggy to make all filters explicit? We don't hide implicit nodes
from query-named-block-nodes (the only interface to explore the whole graph for 
now)
anyway. And we can't absolutely hide side effects of additional node in the 
graph.

So, is there any real benefit of supporting separation into implicit and 
explicit filters?
It seems for me that it only complicates things...
In other words, what will break if we make all filters explicit?

> [...]
> 
 +static int coroutine_fn backup_top_co_flush(BlockDriverState *bs)
 +{
 +if (!bs->backing) {
 +return 0;
 +}
 +
 +return bdrv_co_flush(bs->backing->bs);
>>>
>>> Should we flush the target, too?
>>
>> Hm, you've asked it already, on previous version :)
> 
> I wasn’t sure...
> 
>> Backup don't do it,
>> so I just keep old behavior. And what is the reason to flush backup target
>> on any guest flush?
> 
> Hm, well, what’s the reason not to do it?

guest flushes will be slowed down?

> Also, there are not only guest flushes.  bdrv_flush_all() exists, which
> is called when the guest is stopped.  So who is going to flush the
> target if not its parent?
> 
> [...]

Backup job? But filter should not relay on it..

But should really filter do that job, or it is here only to do CBW? Maybe target
must have another parent which will care about flushing.

Ok, I think I'll flush target here too for safety, and leave a comment, that 
something
smarter would be better.
(or, if we decide to flush all children by default in generic code, I'll drop 
this handler)

> 
 +
 +if (role == _file) {
 +/*
 + * Target child
 + *
 + * Share write to target (child_file), to not interfere
 + * with guest writes to its disk which may be in target backing 
 chain.
 + */
 +if (perm & BLK_PERM_WRITE) {
 +*nshared = *nshared | BLK_PERM_WRITE;
>>>
>>> Why not always share WRITE on the target?
>>
>> Hmm, it's a bad thing to share writes on target, so I'm trying to reduce 
>> number of
>> cases when we have share it.
> 
> Is it?  First of all, this filter doesn’t care.  It doesn’t even read
> from the target (related note: we probably don’t need CONSISTENT_READ on
> the target).
> 
> Second, there is generally going to be a parent on backup-top that has
> the WRITE permission taken.  So this does not really effectively reduce
> that number of cases.

Re: [Qemu-block] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen

[Vladimir Sementsov-Ogievskiy]

14.06.2019 18:52, Max Reitz wrote:
> On 14.06.19 15:42, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> Reopening a node's backing child needs a bit of special handling because
>>> the "backing" child has different defaults than all other children
>>> (among other things).  Adding filter support here is a bit more
>>> difficult than just using the child access functions.  In fact, we often
>>> have to directly use bs->backing because these functions are about the
>>> "backing" child (which may or may not be the COW backing file).
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>block.c | 36 +---
>>>1 file changed, 29 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 505b3e9a01..db2759c10d 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3542,17 +3542,39 @@ static int 
>>> bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>>>}
>>>}
>>>
>>> +/*
>>> + * Ensure that @bs can really handle backing files, because we are
>>> + * about to give it one (or swap the existing one)
>>> + */
>>> +if (bs->drv->is_filter) {
>>> +/* Filters always have a file or a backing child */
>>> +if (!bs->backing) {
>>> +error_setg(errp, "'%s' is a %s filter node that does not 
>>> support a "
>>> +   "backing child", bs->node_name, 
>>> bs->drv->format_name);
>>> +return -EINVAL;
>>> +}
>>> +} else if (!bs->drv->supports_backing) {
>>> +error_setg(errp, "Driver '%s' of node '%s' does not support 
>>> backing "
>>> +   "files", bs->drv->format_name, bs->node_name);
>>> +return -EINVAL;
>>> +}
>>
>> hmm, shouldn't we have these checks for overlay_bs?
> 
> I think this is correct here because this is the only node the user has
> control over, so this is the only one we can reasonably complain about.
> 
> And I do think it is reasonable to complain about.
> 
>>> +
>>>/*
>>> * Find the "actual" backing file by skipping all links that point
>>> * to an implicit node, if any (e.g. a commit filter node).
>>> + * We cannot use any of the bdrv_skip_*() functions here because
>>> + * those return the first explicit node, while we are looking for
>>> + * its overlay here.
>>> */
>>>overlay_bs = bs;
>>> -while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>>> -overlay_bs = backing_bs(overlay_bs);
>>> +while (bdrv_filtered_bs(overlay_bs) &&
>>> +   bdrv_filtered_bs(overlay_bs)->implicit)
>>> +{
>>> +overlay_bs = bdrv_filtered_bs(overlay_bs);
>>>}
>>
>> here, overlay_bs may be some filter with file child ..
>>
>>>
>>>/* If we want to replace the backing file we need some extra checks 
>>> */
>>> -if (new_backing_bs != backing_bs(overlay_bs)) {
>>> +if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) {
>>>/* Check for implicit nodes between bs and its backing file */
>>>if (bs != overlay_bs) {
>>>error_setg(errp, "Cannot change backing link if '%s' has "
>>> @@ -3560,8 +3582,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
>>> *reopen_state,
>>>return -EPERM;
>>>}
>>>/* Check if the backing link that we want to replace is frozen */
>>> -if (bdrv_is_backing_chain_frozen(overlay_bs, 
>>> backing_bs(overlay_bs),
>>> - errp)) {
>>> +if (bdrv_is_backing_chain_frozen(overlay_bs,
>>> + child_bs(overlay_bs->backing), 
>>> errp)) {
>>
>> .. and here we are doing wrong thing, as it don't have backing child
>>
>> Aha, you use the fact that we now don't have implicit filters with file 
>> child. Then, should
>> we add an assertion for this?
> 
> No, that wasn’t my intention.  The real reason is that all of this is a
> mess.
> 
> Here is the full context:
> 
>>  overlay_bs = bs;
>>  while (bdrv_filtered_bs(overlay_bs) &&
>> bdrv_filtered_bs(overlay_bs)->implicit)
>>  {
>>  overlay_bs = bdrv_filtered_bs(overlay_bs);
>>  }
>>
>>  /* If we want to replace the backing file we need some extra checks */
>>  if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) {
>>  /* Check for implicit nodes between bs and its backing file */
>>  if (bs != overlay_bs) {
>>  error_setg(errp, "Cannot change backing link if '%s' has "
>> "an implicit backing file", bs->node_name);
>>  return -EPERM;
>>  }
>>  /* Check if the backing link that we want to replace is frozen */
>>  if (bdrv_is_backing_chain_frozen(overlay_bs,
>>   child_bs(overlay_bs->backing), 
>> errp)) {
>>  return -EPERM;
>>  }
> 
> Note the “Check for implicit nodes” 

Re: [Qemu-block] [PATCH v5 20/42] block/snapshot: Fall back to storage child

[Max Reitz]

On 14.06.19 17:22, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> If the top node's driver does not provide snapshot functionality and we
>> want to go down the chain, we should go towards the child which stores
>> the data, i.e. the storage child.
>>
>> bdrv_snapshot_goto() becomes a bit weird because we may have to redirect
>> the actual child pointer, so it only works if the storage child is
>> bs->file or bs->backing (and then we have to find out which it is).
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block/snapshot.c | 74 ++--
>>   1 file changed, 53 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index f2f48f926a..58cd667f3a 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -154,8 +154,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>>   }
>>   
>>   if (!drv->bdrv_snapshot_create) {
>> -if (bs->file != NULL) {
>> -return bdrv_can_snapshot(bs->file->bs);
>> +BlockDriverState *storage_bs = bdrv_storage_bs(bs);
>> +if (storage_bs) {
>> +return bdrv_can_snapshot(storage_bs);
>>   }
>>   return 0;
>>   }
> 
> Hmm is it correct at all doing a snapshot, when top format node doesn't 
> support it,
> metadata child doesn't support it and storage child supports? Doing snapshots 
> of
> storage child seems useless, as data file must be in sync with metadata.

You’re right.

That’s actually a bug already.  VMDK can store data in multiple
children, but it does not support snapshots.  So if you store such a
split VMDK disk on an RBD volume, it is possible that just the
descriptor file is snapshotted, but nothing else.

Hmmm.  I think the best way is to check whether there is exactly one
child that is not the bdrv_filtered_cow_child().  If so, we can go down
to it and snapshot it.  Otherwise, the node does not support snapshots.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Max Reitz]

On 14.06.19 16:31, Vladimir Sementsov-Ogievskiy wrote:
> 14.06.2019 16:50, Max Reitz wrote:
>> On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
>>> 13.06.2019 1:09, Max Reitz wrote:
 Use child access functions when iterating through backing chains so
 filters do not break the chain.

 Signed-off-by: Max Reitz 
 ---
block.c | 40 
1 file changed, 28 insertions(+), 12 deletions(-)

 diff --git a/block.c b/block.c
 index 11f37983d9..505b3e9a01 100644
 --- a/block.c
 +++ b/block.c
>>
>> [...]
>>
 @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
BlockDriverState *bs)
{
 -while (active && bs != backing_bs(active)) {
 -active = backing_bs(active);
 +bs = bdrv_skip_rw_filters(bs);
 +active = bdrv_skip_rw_filters(active);
 +
 +while (active) {
 +BlockDriverState *next = bdrv_backing_chain_next(active);
 +if (bs == next) {
 +return active;
 +}
 +active = next;
}

 -return active;
 +return NULL;
}
>>>
>>> Semantics changed for this function.
>>> It is used in two places
>>> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we 
>>> will never have
>>>  filter node as a bottom of some valid chain
>>>
>>> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't 
>>> understand,
>>> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs 
>>> overlay is out of the job,
>>> what is this check for?
>>
>> There is a loop before this check which checks that the same blocker is
>> not set on any nodes between top and base (both inclusive).  I guess
>> non-active commit checks the node above @top, too, because its backing
>> file will change.
> 
> So in this case frozen chain works better.

Perhaps.  The op blockers are in this weird state anyway.  I don’t think
we even need them any more, because the permissions were intended to
replace them (they were originally called “fine-grained op blockers”, I
seem to remember).

I dare not touch them.

/* Given a BDS, searches for the base layer. */
>>
>> [...]
>>
 @@ -5149,7 +5165,7 @@ BlockDriverState 
 *bdrv_find_backing_image(BlockDriverState *bs,
char *backing_file_full_ret;

if (strcmp(backing_file, curr_bs->backing_file) == 0) {
>>>
>>> hmm, interesting, what bs->backing_file now means? It's strange enough to 
>>> store such field on
>>> bds, when we have backing link anyway..
>>
>> Patch 37 has you covered. :-)
>>
> 
> Hmm, if it has removed this field, but it doesn't)

Because it’s needed.  (Just not in the current form, but that’s what 37
is for.)

> So, we finished with some object, called "overlay", but it is not an overlay 
> of bs, it's overlay of
> first non-implicit filtered node in bs backing chain, it may be found by 
> bdrv_find_overlay() helper (which is
> almost unused and my be safely dropped), and filename of this "overlay" is 
> stored in bs->backing_file string
> variable, keeping in mind that bs->backing is pointer to backing child of bs 
> which is completely another thing?

I don’t quite see what you mean.  There is no “overlay” in this function.

> Oh, no, everything related to filename-based backing chain logic is not for 
> me o_O. If something doesn't work
> with filename-based logic users should use node-names..

In theory yes, but that isn’t an option for qemu-img commit, for example.

> And I'd prefer to deprecate filename based interfaces at all.

Me too.

https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04878.html

:-/

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 23/42] blockdev: Use CAF in external_snapshot_prepare()

[Max Reitz]

On 14.06.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> This allows us to differentiate between filters and nodes with COW
>> backing files: Filters cannot be used as overlays at all (for this
>> function).
>>
>> Signed-off-by: Max Reitz 
> 
> Overlay created in snapshot operation assumed to consume following writes
> and it's filtered child becomes readonly.. And filter works in completely 
> another
> way.
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
> [hmm, I start to like using "filtered child" collocation when I say about 
> this thing.
>   didn't you think about renaming backing chain to filtered chain?]

Hm.  There are backing chains and there are backing chains.  There are
qemu-internal backing chains that consist of a healthy mix of filters
and COW overlays, and then there are the more high-level backing chains
the user actually manages, where only the overlays are important.

I think it would make sense to rename the “qemu-internal backing chains"
to “filter chains” or something.  But that makes it sound a bit like it
would only mean R/W filters...  Maybe just “chain”?

Actually, the only functions I find are is_backing_chain_frozen & Co,
and they could simply become is_chain_frozen.  Is there anything else?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 16/42] block: Use child access functions when flushing

[Max Reitz]

On 14.06.19 16:01, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> If the driver does not support .bdrv_co_flush() so bdrv_co_flush()
>> itself has to flush the children of the given node, it should not flush
>> just bs->file->bs, but in fact both the child that stores data, and the
>> one that stores metadata (if they are separate).
>>
>> In any case, the BLKDBG_EVENT() should be emitted on the primary child,
>> because that is where a blkdebug node would be if there is any.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block/io.c | 21 ++---
>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 53aabf86b5..64408cf19a 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -2533,6 +2533,8 @@ static void coroutine_fn bdrv_flush_co_entry(void 
>> *opaque)
>>   
>>   int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>>   {
>> +BdrvChild *primary_child = bdrv_primary_child(bs);
>> +BlockDriverState *storage_bs, *metadata_bs;
>>   int current_gen;
>>   int ret = 0;
>>   
>> @@ -2562,7 +2564,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>>   }
>>   
>>   /* Write back cached data to the OS even with cache=unsafe */
>> -BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS);
>> +BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_OS);
>>   if (bs->drv->bdrv_co_flush_to_os) {
>>   ret = bs->drv->bdrv_co_flush_to_os(bs);
>>   if (ret < 0) {
>> @@ -2580,7 +2582,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>>   goto flush_parent;
>>   }
>>   
>> -BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
>> +BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_DISK);
>>   if (!bs->drv) {
>>   /* bs->drv->bdrv_co_flush() might have ejected the BDS
>>* (even in case of apparent success) */
>> @@ -2625,7 +2627,20 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>>* in the case of cache=unsafe, so there are no useless flushes.
>>*/
>>   flush_parent:
>> -ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
>> +storage_bs = bdrv_storage_bs(bs);
>> +metadata_bs = bdrv_metadata_bs(bs);
>> +
>> +ret = 0;
>> +if (storage_bs) {
>> +ret = bdrv_co_flush(storage_bs);
>> +}
>> +if (metadata_bs && metadata_bs != storage_bs) {
>> +int ret_metadata = bdrv_co_flush(metadata_bs);
>> +if (!ret) {
>> +ret = ret_metadata;
>> +}
>> +}
>> +
>>   out:
>>   /* Notify any pending flushes that we have completed */
>>   if (ret == 0) {
>>
> 
> Hmm, I'm not sure that if in one driver we decided to store data and metadata 
> separately,
> we need to support flushing them both generic code.. If at some point qcow2 
> decides store part
> of metadata in third child, we will not flush it here too?
> 
> Should not we instead loop through children and flush all? And I'd 
> s/flush_parent/flush_children as
> it is rather weird.

That sounds good.  Well, we only need to flush the ones the driver has
taken a WRITE permission on, but yes.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 21/42] block: Use CAFs for debug breakpoints

[Max Reitz]

On 14.06.19 17:29, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> When looking for a blkdebug node (which implements debug breakpoints),
>> use bdrv_primary_bs() to iterate through the graph, because that is
>> where a blkdebug node would be.
>>
>> Signed-off-by: Max Reitz 
> 
> Honestly, don't know why blkdebug is always searched in ->file sequence,

Usually, blkdebug is just above the protocol node.  So

$format --file--> $protocol

becomes

$format --file--> blkdebug --file--> $protocol

This is why the existing code generally looks for blkdebug under the
->file link.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 22/42] block: Use CAFs in bdrv_get_allocated_file_size()

[Max Reitz]

On 14.06.19 17:41, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>   block.c | 26 --
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 11b7ba8cf6..856d9b58be 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4511,15 +4511,37 @@ exit:
>>   int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>>   {
>>   BlockDriver *drv = bs->drv;
>> +BlockDriverState *storage_bs, *metadata_bs;
>> +
>>   if (!drv) {
>>   return -ENOMEDIUM;
>>   }
>> +
>>   if (drv->bdrv_get_allocated_file_size) {
>>   return drv->bdrv_get_allocated_file_size(bs);
>>   }
>> -if (bs->file) {
>> -return bdrv_get_allocated_file_size(bs->file->bs);
>> +
>> +storage_bs = bdrv_storage_bs(bs);
>> +metadata_bs = bdrv_metadata_bs(bs);
>> +
>> +if (storage_bs) {
>> +int64_t data_size, metadata_size = 0;
>> +
>> +data_size = bdrv_get_allocated_file_size(storage_bs);
>> +if (data_size < 0) {
>> +return data_size;
>> +}
>> +
>> +if (storage_bs != metadata_bs) {
>> +metadata_size = bdrv_get_allocated_file_size(metadata_bs);
>> +if (metadata_size < 0) {
>> +return metadata_size;
>> +}
>> +}
>> +
>> +return data_size + metadata_size;
>>   }
>> +
>>   return -ENOTSUP;
>>   }
>>   
>>
> 
> Again, I dislike nailing down new fresh feature about separate metadata and 
> storage child
> to the generic block layer, as it's simple to imagine a driver which needs 
> three or more
> children to store all its data and metadata..

Yes, we have that, it’s VMDK.

> Isn't it better by default loop through all children and sum all their 
> allocated sizes?
> 
> Hmm, but we want exclude backing, yes? Still we may ignore it while iterating.

I want to object in that there could be drivers that have children that
should not count towards their allocated size other than COW backing
files.  But I actually cannot imagine a reasonable scenario.  (The only
reason why COW backing files should be excluded is because they are
generally listed separately.)

So, yes, that sounds good.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 0/4] block: Keep track of parent quiescing

[Kevin Wolf]

Am 05.06.2019 um 18:11 hat Max Reitz geschrieben:
> We currently do not keep track of how many times a child has quiesced
> its parent.  We just guess based on the child’s quiesce_counter.  That
> keeps biting us when we try to leave drained sections or detach children
> (see e.g. commit 5cb2737e925042e).
> 
> I think we need an explicit counter to keep track of how many times a
> parent has been quiesced (patch 1).  That just makes the code more
> resilient.
> 
> Actually, no, we don’t need a counter, we need a boolean.  See patch 2
> for the explanation.
> 
> Yes, it is a bit weird to introduce a counter first (patch 1) and then
> immediately make it a boolean (patch 2).  But I believe this to be the
> most logical change set.
> 
> (“Our current model relies on counting, so adding an explicit counter
> makes sense.  It then turns out that counting is probably not the best
> idea, so why not make it a boolean?”)

Trying to summarise an IRC discussion I just had with Max:

The real root problem isn't that the recursion in bdrv_do_drained_end()
doesn't correctly deal with graph changes, but that those graph changes
happen in the first place. The one basic guiding principle in my drain
rewrite was that during the recursion (both to children and parents), no
graph changes are allowed, which means that no aio_poll() calls are
allowed either.

Of course, while I think the principle is right and greatly simplifies
the code (or actually is the only thing that gives us any hope to get
things right), I messed up the implementation because
bdrv_drain_invoke() does use BDRV_POLL_WHILE() for ending a drained
section. This is wrong, and it could still cause crashes after this
series because a recursive call could remove a node that is currently
processed somewhere up the call chain.

The fix for the observed bugs should be to make drained_end completely
symmetric to drained_begin: Just start the bdrv_drain_invoke_entry()
coroutine, do the recursion and call all the callbacks (none of which
may modify the graph) and only after all of this is done, poll once at
the top level drain. (The poll condition could be simplified to just
wait for bdrv_drain_invoke() to be completed, we don't care about other
running requests in drained_end. But this is only an optimisation.)

Despite this being a full fix, we also agreed that patch 1 is a nice
cleanup and we want to keep it even if it doesn't strictly fix a bug any
more.

Kevin

Re: [Qemu-block] [PATCH v5 21/42] block: Use CAFs for debug breakpoints

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> When looking for a blkdebug node (which implements debug breakpoints),
> use bdrv_primary_bs() to iterate through the graph, because that is
> where a blkdebug node would be.
> 
> Signed-off-by: Max Reitz 

Honestly, don't know why blkdebug is always searched in ->file sequence, but 
this
patch obviously supports backing-based filters for blkdebug scenarios, which I 
need
for my backup-top series (and have corresponding patch in it, which is not 
needed
if this goes first)


Reviewed-by: Vladimir Sementsov-Ogievskiy 


> ---
>   block.c | 8 
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 797bec0326..11b7ba8cf6 100644
> --- a/block.c
> +++ b/block.c
> @@ -5097,7 +5097,7 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const 
> char *event,
> const char *tag)
>   {
>   while (bs && bs->drv && !bs->drv->bdrv_debug_breakpoint) {
> -bs = bs->file ? bs->file->bs : NULL;
> +bs = bdrv_primary_bs(bs);
>   }
>   
>   if (bs && bs->drv && bs->drv->bdrv_debug_breakpoint) {
> @@ -5110,7 +5110,7 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const 
> char *event,
>   int bdrv_debug_remove_breakpoint(BlockDriverState *bs, const char *tag)
>   {
>   while (bs && bs->drv && !bs->drv->bdrv_debug_remove_breakpoint) {
> -bs = bs->file ? bs->file->bs : NULL;
> +bs = bdrv_primary_bs(bs);
>   }
>   
>   if (bs && bs->drv && bs->drv->bdrv_debug_remove_breakpoint) {
> @@ -5123,7 +5123,7 @@ int bdrv_debug_remove_breakpoint(BlockDriverState *bs, 
> const char *tag)
>   int bdrv_debug_resume(BlockDriverState *bs, const char *tag)
>   {
>   while (bs && (!bs->drv || !bs->drv->bdrv_debug_resume)) {
> -bs = bs->file ? bs->file->bs : NULL;
> +bs = bdrv_primary_bs(bs);
>   }
>   
>   if (bs && bs->drv && bs->drv->bdrv_debug_resume) {
> @@ -5136,7 +5136,7 @@ int bdrv_debug_resume(BlockDriverState *bs, const char 
> *tag)
>   bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
>   {
>   while (bs && bs->drv && !bs->drv->bdrv_debug_is_suspended) {
> -bs = bs->file ? bs->file->bs : NULL;
> +bs = bdrv_primary_bs(bs);
>   }
>   
>   if (bs && bs->drv && bs->drv->bdrv_debug_is_suspended) {
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 22/42] block: Use CAFs in bdrv_get_allocated_file_size()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>   block.c | 26 --
>   1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 11b7ba8cf6..856d9b58be 100644
> --- a/block.c
> +++ b/block.c
> @@ -4511,15 +4511,37 @@ exit:
>   int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>   {
>   BlockDriver *drv = bs->drv;
> +BlockDriverState *storage_bs, *metadata_bs;
> +
>   if (!drv) {
>   return -ENOMEDIUM;
>   }
> +
>   if (drv->bdrv_get_allocated_file_size) {
>   return drv->bdrv_get_allocated_file_size(bs);
>   }
> -if (bs->file) {
> -return bdrv_get_allocated_file_size(bs->file->bs);
> +
> +storage_bs = bdrv_storage_bs(bs);
> +metadata_bs = bdrv_metadata_bs(bs);
> +
> +if (storage_bs) {
> +int64_t data_size, metadata_size = 0;
> +
> +data_size = bdrv_get_allocated_file_size(storage_bs);
> +if (data_size < 0) {
> +return data_size;
> +}
> +
> +if (storage_bs != metadata_bs) {
> +metadata_size = bdrv_get_allocated_file_size(metadata_bs);
> +if (metadata_size < 0) {
> +return metadata_size;
> +}
> +}
> +
> +return data_size + metadata_size;
>   }
> +
>   return -ENOTSUP;
>   }
>   
> 

Again, I dislike nailing down new fresh feature about separate metadata and 
storage child
to the generic block layer, as it's simple to imagine a driver which needs 
three or more
children to store all its data and metadata..

Isn't it better by default loop through all children and sum all their 
allocated sizes?

Hmm, but we want exclude backing, yes? Still we may ignore it while iterating.

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen

[Max Reitz]

On 14.06.19 15:42, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Reopening a node's backing child needs a bit of special handling because
>> the "backing" child has different defaults than all other children
>> (among other things).  Adding filter support here is a bit more
>> difficult than just using the child access functions.  In fact, we often
>> have to directly use bs->backing because these functions are about the
>> "backing" child (which may or may not be the COW backing file).
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block.c | 36 +---
>>   1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 505b3e9a01..db2759c10d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3542,17 +3542,39 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
>> *reopen_state,
>>   }
>>   }
>>   
>> +/*
>> + * Ensure that @bs can really handle backing files, because we are
>> + * about to give it one (or swap the existing one)
>> + */
>> +if (bs->drv->is_filter) {
>> +/* Filters always have a file or a backing child */
>> +if (!bs->backing) {
>> +error_setg(errp, "'%s' is a %s filter node that does not 
>> support a "
>> +   "backing child", bs->node_name, 
>> bs->drv->format_name);
>> +return -EINVAL;
>> +}
>> +} else if (!bs->drv->supports_backing) {
>> +error_setg(errp, "Driver '%s' of node '%s' does not support backing 
>> "
>> +   "files", bs->drv->format_name, bs->node_name);
>> +return -EINVAL;
>> +}
> 
> hmm, shouldn't we have these checks for overlay_bs?

I think this is correct here because this is the only node the user has
control over, so this is the only one we can reasonably complain about.

And I do think it is reasonable to complain about.

>> +
>>   /*
>>* Find the "actual" backing file by skipping all links that point
>>* to an implicit node, if any (e.g. a commit filter node).
>> + * We cannot use any of the bdrv_skip_*() functions here because
>> + * those return the first explicit node, while we are looking for
>> + * its overlay here.
>>*/
>>   overlay_bs = bs;
>> -while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>> -overlay_bs = backing_bs(overlay_bs);
>> +while (bdrv_filtered_bs(overlay_bs) &&
>> +   bdrv_filtered_bs(overlay_bs)->implicit)
>> +{
>> +overlay_bs = bdrv_filtered_bs(overlay_bs);
>>   }
> 
> here, overlay_bs may be some filter with file child ..
> 
>>   
>>   /* If we want to replace the backing file we need some extra checks */
>> -if (new_backing_bs != backing_bs(overlay_bs)) {
>> +if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) {
>>   /* Check for implicit nodes between bs and its backing file */
>>   if (bs != overlay_bs) {
>>   error_setg(errp, "Cannot change backing link if '%s' has "
>> @@ -3560,8 +3582,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
>> *reopen_state,
>>   return -EPERM;
>>   }
>>   /* Check if the backing link that we want to replace is frozen */
>> -if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
>> - errp)) {
>> +if (bdrv_is_backing_chain_frozen(overlay_bs,
>> + child_bs(overlay_bs->backing), 
>> errp)) {
> 
> .. and here we are doing wrong thing, as it don't have backing child
> 
> Aha, you use the fact that we now don't have implicit filters with file 
> child. Then, should
> we add an assertion for this?

No, that wasn’t my intention.  The real reason is that all of this is a
mess.

Here is the full context:

> overlay_bs = bs;
> while (bdrv_filtered_bs(overlay_bs) &&
>bdrv_filtered_bs(overlay_bs)->implicit)
> {
> overlay_bs = bdrv_filtered_bs(overlay_bs);
> }
> 
> /* If we want to replace the backing file we need some extra checks */
> if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) {
> /* Check for implicit nodes between bs and its backing file */
> if (bs != overlay_bs) {
> error_setg(errp, "Cannot change backing link if '%s' has "
>   
>   
>
>"an implicit backing file", bs->node_name);
> return -EPERM;
> }
> /* Check if the backing link that we want to replace is frozen */
> if (bdrv_is_backing_chain_frozen(overlay_bs,
>  child_bs(overlay_bs->backing), 
> errp)) {
> return -EPERM;
> }

Note the “Check for implicit nodes” thing.  If we get to the frozen

Re: [Qemu-block] [Qemu-devel] [PATCH 0/6] configure: Try to fix --static linking

[Philippe Mathieu-Daudé]

On 6/14/19 4:30 PM, Peter Maydell wrote:
> On Fri, 14 Jun 2019 at 14:58, Alex Bennée  wrote:
> 
>> It would be nice to have a --static-user config flag and deprecate the
>> --static flag. I don't think there is a decent use case for system
>> emulation targets.
> 
> It would be really tricky to build half with static and half
> without: our configure and build system really assumes that
> fundamental stuff like "what libraries" and "what compiler flags"
> are the same across the whole of the build.
> 
> Is --static-user really much better than:
>  * allow --static --disable-system --disable-tools
>  * forbid --static without --disable-system --disable-tools
>  * require users to build the static usermode binaries separately
>from the system/tools build
> 
> (which is in practice what we have now) ?
> 
> Debian wants both static usermode and non-static usermode
> binaries, so they'd still need to build multiple times anyway.

Glad to read, so the v2 of this series is worthful.

Re: [Qemu-block] [PATCH v5 23/42] blockdev: Use CAF in external_snapshot_prepare()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> This allows us to differentiate between filters and nodes with COW
> backing files: Filters cannot be used as overlays at all (for this
> function).
> 
> Signed-off-by: Max Reitz 

Overlay created in snapshot operation assumed to consume following writes
and it's filtered child becomes readonly.. And filter works in completely 
another
way.

Reviewed-by: Vladimir Sementsov-Ogievskiy 

[hmm, I start to like using "filtered child" collocation when I say about this 
thing.
  didn't you think about renaming backing chain to filtered chain?]

> ---
>   blockdev.c | 7 ++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b5c0fd3c49..0f0cf0d9ae 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1665,7 +1665,12 @@ static void external_snapshot_prepare(BlkActionState 
> *common,
>   goto out;
>   }
>   
> -if (state->new_bs->backing != NULL) {
> +if (state->new_bs->drv->is_filter) {
> +error_setg(errp, "Filters cannot be used as overlays");
> +goto out;
> +}
> +
> +if (bdrv_filtered_cow_child(state->new_bs)) {
>   error_setg(errp, "The overlay already has a backing image");
>   goto out;
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/14/19 4:30 PM, Max Reitz wrote:
> On 14.06.19 16:26, Philippe Mathieu-Daudé wrote:
>> On 6/13/19 9:31 PM, Max Reitz wrote:
>>> On 13.06.19 15:20, Pino Toscano wrote:
 Rewrite the implementation of the ssh block driver to use libssh instead
 of libssh2.  The libssh library has various advantages over libssh2:
 - easier API for authentication (for example for using ssh-agent)
 - easier API for known_hosts handling
 - supports newer types of keys in known_hosts

 Use APIs/features available in libssh 0.8 conditionally, to support
 older versions (which are not recommended though).

 Adjust the tests according to the different error message, and to the
 newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
 and libssh >= 0.7.0.

 Adjust the various Docker/Travis scripts to use libssh when available
 instead of libssh2. The mingw/mxe testing is dropped for now, as there
 are no packages for it.

 Signed-off-by: Pino Toscano 
 Tested-by: Philippe Mathieu-Daudé 
 Acked-by: Alex Bennée 
 ---
>>>
>>> Can confirm that this runs much faster than the last version I tested.
>>> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
>>> with me. :-)
>>>
 Changes from v8:
 - use a newer key type in iotest 207
 - improve the commit message

 Changes from v7:
 - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
 - ptrdiff_t -> size_t

 Changes from v6:
 - fixed few checkpatch style issues
 - detect libssh 0.8 via symbol detection
 - adjust travis/docker test material
 - remove dead "default" case in a switch
 - use variables for storing MIN() results
 - adapt a documentation bit

 Changes from v5:
 - adapt to newer tracing APIs
 - disable ssh compression (mimic what libssh2 does by default)
 - use build time checks for libssh 0.8, and use newer APIs directly

 Changes from v4:
 - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
 - fix few return code checks
 - remove now-unused parameters in few internal functions
 - allow authentication with "none" method
 - switch to unsigned int for the port number
 - enable TCP_NODELAY on the socket
 - fix one reference error message in iotest 207

 Changes from v3:
 - fix socket cleanup in connect_to_ssh()
 - add comments about the socket cleanup
 - improve the error reporting (closer to what was with libssh2)
 - improve EOF detection on sftp_read()

 Changes from v2:
 - used again an own fd
 - fixed co_yield() implementation

 Changes from v1:
 - fixed jumbo packets writing
 - fixed missing 'err' assignment
 - fixed commit message

  .travis.yml   |   4 +-
  block/Makefile.objs   |   6 +-
  block/ssh.c   | 622 +-
  block/trace-events|  14 +-
  configure |  65 +-
  docs/qemu-block-drivers.texi  |   2 +-
  .../dockerfiles/debian-win32-cross.docker |   1 -
  .../dockerfiles/debian-win64-cross.docker |   1 -
  tests/docker/dockerfiles/fedora.docker|   4 +-
  tests/docker/dockerfiles/ubuntu.docker|   2 +-
  tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
  tests/qemu-iotests/207|   4 +-
  tests/qemu-iotests/207.out|   2 +-
  13 files changed, 376 insertions(+), 353 deletions(-)
>>>
>>> Surprisingly little has changed since v4.  Good, good, that makes my
>>> reviewing job much easier because I can thus rely on past me. :-)
>>>
 diff --git a/block/ssh.c b/block/ssh.c
 index 6da7b9cbfe..fb458d4548 100644
 --- a/block/ssh.c
 +++ b/block/ssh.c
>>>
>>> [...]
>>>
 @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename, 
 QDict *options,
  parse_uri(filename, options, errp);
  }
  
 -static int check_host_key_knownhosts(BDRVSSHState *s,
 - const char *host, int port, Error 
 **errp)
 +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
  {
>>>
>>> [...]
>>>
 -switch (r) {
 -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
 +switch (state) {
 +case SSH_KNOWN_HOSTS_OK:
  /* OK */
 -trace_ssh_check_host_key_knownhosts(found->key);
 +trace_ssh_check_host_key_knownhosts();
  break;
 -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
 +case SSH_KNOWN_HOSTS_CHANGED:
  ret = -EINVAL;
 -session_error_setg(errp, s,
 -  "host key does not match the one in known_hosts"
 -  " (found key %s)", found->key);
 +

Re: [Qemu-block] [PATCH v5 20/42] block/snapshot: Fall back to storage child

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> If the top node's driver does not provide snapshot functionality and we
> want to go down the chain, we should go towards the child which stores
> the data, i.e. the storage child.
> 
> bdrv_snapshot_goto() becomes a bit weird because we may have to redirect
> the actual child pointer, so it only works if the storage child is
> bs->file or bs->backing (and then we have to find out which it is).
> 
> Signed-off-by: Max Reitz 
> ---
>   block/snapshot.c | 74 ++--
>   1 file changed, 53 insertions(+), 21 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index f2f48f926a..58cd667f3a 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -154,8 +154,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>   }
>   
>   if (!drv->bdrv_snapshot_create) {
> -if (bs->file != NULL) {
> -return bdrv_can_snapshot(bs->file->bs);
> +BlockDriverState *storage_bs = bdrv_storage_bs(bs);
> +if (storage_bs) {
> +return bdrv_can_snapshot(storage_bs);
>   }
>   return 0;
>   }

Hmm is it correct at all doing a snapshot, when top format node doesn't support 
it,
metadata child doesn't support it and storage child supports? Doing snapshots of
storage child seems useless, as data file must be in sync with metadata.


> @@ -167,14 +168,15 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>QEMUSnapshotInfo *sn_info)
>   {
>   BlockDriver *drv = bs->drv;
> +BlockDriverState *storage_bs = bdrv_storage_bs(bs);
>   if (!drv) {
>   return -ENOMEDIUM;
>   }
>   if (drv->bdrv_snapshot_create) {
>   return drv->bdrv_snapshot_create(bs, sn_info);
>   }
> -if (bs->file) {
> -return bdrv_snapshot_create(bs->file->bs, sn_info);
> +if (storage_bs) {
> +return bdrv_snapshot_create(storage_bs, sn_info);
>   }
>   return -ENOTSUP;
>   }
> @@ -184,6 +186,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>  Error **errp)
>   {
>   BlockDriver *drv = bs->drv;
> +BlockDriverState *storage_bs;
>   int ret, open_ret;
>   
>   if (!drv) {
> @@ -204,39 +207,66 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>   return ret;
>   }
>   
> -if (bs->file) {
> -BlockDriverState *file;
> -QDict *options = qdict_clone_shallow(bs->options);
> +storage_bs = bdrv_storage_bs(bs);
> +if (storage_bs) {
> +QDict *options;
>   QDict *file_options;
>   Error *local_err = NULL;
> +bool is_backing_child;
> +BdrvChild **child_pointer;
> +
> +/*
> + * Filters may reference the storage child through
> + * bs->backing.  We need to know whether we are dealing with
> + * bs->backing or bs->file, so we check it here.
> + */
> +if (storage_bs == bs->file->bs) {
> +is_backing_child = false;
> +child_pointer = >file;
> +} else if (storage_bs == bs->backing->bs) {
> +is_backing_child = true;
> +child_pointer = >backing;
> +} else {
> +/*
> + * The storage child is not referenced by a field in the
> + * BDS object.  We cannot go on then.
> + */
> +error_setg(errp, "Block driver does not support snapshots");
> +return -ENOTSUP;
> +}
> +
> +options = qdict_clone_shallow(bs->options);
>   
> -file = bs->file->bs;
>   /* Prevent it from getting deleted when detached from bs */
> -bdrv_ref(file);
> +bdrv_ref(storage_bs);
>   
> -qdict_extract_subqdict(options, _options, "file.");
> +qdict_extract_subqdict(options, _options,
> +   is_backing_child ? "backing." : "file.");
>   qobject_unref(file_options);
> -qdict_put_str(options, "file", bdrv_get_node_name(file));
> +qdict_put_str(options, is_backing_child ? "backing" : "file",
> +  bdrv_get_node_name(storage_bs));
>   
>   if (drv->bdrv_close) {
>   drv->bdrv_close(bs);
>   }
> -bdrv_unref_child(bs, bs->file);
> -bs->file = NULL;
>   
> -ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> +assert(storage_bs == (*child_pointer)->bs);
> +bdrv_unref_child(bs, *child_pointer);
> +*child_pointer = NULL;
> +
> +ret = bdrv_snapshot_goto(storage_bs, snapshot_id, errp);
>   open_ret = drv->bdrv_open(bs, options, bs->open_flags, _err);
>   qobject_unref(options);
>   if (open_ret < 0) {
> -bdrv_unref(file);
> +bdrv_unref(storage_bs);
>   bs->drv = NULL;
>   /* A bdrv_snapshot_goto() error takes precedence */
>   error_propagate(errp, local_err);
>   return ret < 0 ? ret 

Re: [Qemu-block] [PATCH v5 19/42] block: Use CAF in bdrv_co_rw_vmstate()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> If a node whose driver does not provide VM state functions has a
> metadata child, the VM state should probably go there; if it is a
> filter, the VM state should probably go there.  It follows that we
> should generally go down to the primary child.

Hmm, as I understand vmstate is something stored in file and invisible for 
actual file user,
which may be guest or format node.. So actually it doesn't matter in which
child to store it, it should be transparent for the parent.. Maybe the right
thing is to loop through children and use first which supports storing vmstate.

But I'm OK with this too.

(hmm you assume that vmstate should go to metadata child,
but the only format which has separate metadata and storage child stores 
vmstate to
storage child)

Reviewed-by: Vladimir Sementsov-Ogievskiy 


> 
> Signed-off-by: Max Reitz 
> ---
>   block/io.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 659ea0c52a..14f99e1c00 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2395,6 +2395,7 @@ bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector 
> *qiov, int64_t pos,
>  bool is_read)
>   {
>   BlockDriver *drv = bs->drv;
> +BlockDriverState *child_bs = bdrv_primary_bs(bs);
>   int ret = -ENOTSUP;
>   
>   bdrv_inc_in_flight(bs);
> @@ -2407,8 +2408,8 @@ bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector 
> *qiov, int64_t pos,
>   } else {
>   ret = drv->bdrv_save_vmstate(bs, qiov, pos);
>   }
> -} else if (bs->file) {
> -ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
> +} else if (child_bs) {
> +ret = bdrv_co_rw_vmstate(child_bs, qiov, pos, is_read);
>   }
>   
>   bdrv_dec_in_flight(bs);
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/14/19 4:34 PM, Max Reitz wrote:
> On 14.06.19 16:29, Pino Toscano wrote:
>> On Thursday, 13 June 2019 21:31:40 CEST Max Reitz wrote:
>>> On 13.06.19 15:20, Pino Toscano wrote:
[...]
 -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
 +case SSH_KNOWN_HOSTS_OTHER:
  ret = -EINVAL;
 -session_error_setg(errp, s, "no host key was found in 
 known_hosts");
 +error_setg(errp,
 +   "host key for this server not found, another type 
 exists");
  goto out;
 -case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
 +case SSH_KNOWN_HOSTS_UNKNOWN:
  ret = -EINVAL;
 -session_error_setg(errp, s,
 -  "failure matching the host key with known_hosts");
 +error_setg(errp, "no host key was found in known_hosts");
 +goto out;
 +case SSH_KNOWN_HOSTS_NOT_FOUND:
 +ret = -ENOENT;
 +error_setg(errp, "known_hosts file not found");
 +goto out;
 +case SSH_KNOWN_HOSTS_ERROR:
 +ret = -EINVAL;
 +error_setg(errp, "error while checking the host");
  goto out;
  default:
  ret = -EINVAL;
 -session_error_setg(errp, s, "unknown error matching the host key"
 -  " with known_hosts (%d)", r);
 +error_setg(errp, "error while checking for known server");
  goto out;
  }
 +#else /* !HAVE_LIBSSH_0_8 */
 +int state;
 +
 +state = ssh_is_server_known(s->session);
 +trace_ssh_server_status(state);
 +
 +switch (state) {
 +case SSH_SERVER_KNOWN_OK:
 +/* OK */
 +trace_ssh_check_host_key_knownhosts();
 +break;
 +case SSH_SERVER_KNOWN_CHANGED:
 +ret = -EINVAL;
 +error_setg(errp, "host key does not match the one in 
 known_hosts");
 +goto out;
 +case SSH_SERVER_FOUND_OTHER:
 +ret = -EINVAL;
 +error_setg(errp,
 +   "host key for this server not found, another type 
 exists");
 +goto out;
 +case SSH_SERVER_FILE_NOT_FOUND:
 +ret = -ENOENT;
 +error_setg(errp, "known_hosts file not found");
 +goto out;
 +case SSH_SERVER_NOT_KNOWN:
 +ret = -EINVAL;
 +error_setg(errp, "no host key was found in known_hosts");
 +goto out;
 +case SSH_SERVER_ERROR:
 +ret = -EINVAL;
 +error_setg(errp, "server error");
 +goto out;
>>>
>>> No default here?
>>
>> This switch is for libssh < 0.8.0, so enumerating all the possible
>> values of the enum of the old API is enough.
> 
> state is an integer.  I feel very uneasy about not having a default
> clause for a plain integer, especially if it is supplied by an external
> library.

Agreed. What's odd is I tested it on Ubuntu Xenial which is 0.6.3 and no
got no cpp warning. I wonder if it is using a backported patch adding
ssh_session_is_known_server(), like 0.7.1 on Ubuntu Bionic. Anyway,
better add a default.



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/13/19 8:18 PM, Paolo Bonzini wrote:
> On 13/06/19 19:15, Philippe Mathieu-Daudé wrote:
>>
>> On 6/13/19 6:24 PM, no-re...@patchew.org wrote:
>>> === TEST SCRIPT BEGIN ===
>>> #!/bin/bash
>>> time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
>>> === TEST SCRIPT END ===
>>
>>> The full log is available at
>>> http://patchew.org/logs/20190613132000.2146-1-ptosc...@redhat.com/testing.asan/?type=message.
>>
>>
>> === OUTPUT BEGIN ===
>>   BUILD   fedora
>> The command '/bin/sh -c dnf install -y $PACKAGES' returned a non-zero
>> code: 1
>> Traceback (most recent call last):
>>   File "./tests/docker/docker.py", line 615, in 
>> sys.exit(main())
>>   File "./tests/docker/docker.py", line 611, in main
>> return args.cmdobj.run(args, argv)
>>   File "./tests/docker/docker.py", line 413, in run
>> extra_files_cksum=cksum)
>>   File "./tests/docker/docker.py", line 280, in build_image
>> quiet=quiet)
>>   File "./tests/docker/docker.py", line 207, in _do_check
>> return subprocess.check_call(self._command + cmd, **kwargs)
>>   File "/usr/lib64/python2.7/subprocess.py", line 542, in check_call
>> raise CalledProcessError(retcode, cmd)
>> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker',
>> 'build', '-t', 'qemu:fedora', '-f',
>> '/tmp/docker_buildN2FMKc/tmpz_9Up_.docker', '/tmp/docker_buildN2FMKc']'
>> returned non-zero exit status 1
>> make: *** [docker-image-fedora] Error 1
>>
>> real 3m54.376s
>>
>> Not sure this is a network issue or something else, should we rebuild
>> docker images with V=1 on patchew?
>>
> 
> I restarted the job with V=1.

Thanks!

Patchew did not send update, which means no failure.
Hopefully it updated the same url with the full log.
Indeed the job is successful:


real30m39.367s
user0m8.061s
sys 0m5.880s
=== OUTPUT END ===

Test command exited with code: 0

Re: [Qemu-block] [PATCH v5 17/42] block: Use CAFs in bdrv_refresh_limits()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy 

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PULL 00/20] Block patches

[Peter Maydell]

On Fri, 14 Jun 2019 at 14:40, Max Reitz  wrote:
>
> The following changes since commit 5ec2eca83dc478ddf24077e02a8b34dd26cd3ff9:
>
>   Merge remote-tracking branch 
> 'remotes/awilliam/tags/vfio-updates-20190613.0' into staging (2019-06-14 
> 09:33:55 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2019-06-14
>
> for you to fetch changes up to 21c1ce592a144188dfe59b9e156a97da412a59a2:
>
>   iotests: Test qemu-img convert -C --salvage (2019-06-14 15:09:42 +0200)
>
> 
> Block patches:
> - Allow blockdev-backup from nodes that are not in qemu's main AIO
>   context to newly added nodes
> - Add salvaging mode to qemu-img convert
> - Minor fixes to tests, documentation, and for less Valgrind annoyance
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM

Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Vladimir Sementsov-Ogievskiy]

14.06.2019 16:50, Max Reitz wrote:
> On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 1:09, Max Reitz wrote:
>>> Use child access functions when iterating through backing chains so
>>> filters do not break the chain.
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>block.c | 40 
>>>1 file changed, 28 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 11f37983d9..505b3e9a01 100644
>>> --- a/block.c
>>> +++ b/block.c
> 
> [...]
> 
>>> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>>BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>>>BlockDriverState *bs)
>>>{
>>> -while (active && bs != backing_bs(active)) {
>>> -active = backing_bs(active);
>>> +bs = bdrv_skip_rw_filters(bs);
>>> +active = bdrv_skip_rw_filters(active);
>>> +
>>> +while (active) {
>>> +BlockDriverState *next = bdrv_backing_chain_next(active);
>>> +if (bs == next) {
>>> +return active;
>>> +}
>>> +active = next;
>>>}
>>>
>>> -return active;
>>> +return NULL;
>>>}
>>
>> Semantics changed for this function.
>> It is used in two places
>> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we 
>> will never have
>>  filter node as a bottom of some valid chain
>>
>> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't 
>> understand,
>> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs 
>> overlay is out of the job,
>> what is this check for?
> 
> There is a loop before this check which checks that the same blocker is
> not set on any nodes between top and base (both inclusive).  I guess
> non-active commit checks the node above @top, too, because its backing
> file will change.

So in this case frozen chain works better.

> 
>>>/* Given a BDS, searches for the base layer. */
> 
> [...]
> 
>>> @@ -5149,7 +5165,7 @@ BlockDriverState 
>>> *bdrv_find_backing_image(BlockDriverState *bs,
>>>char *backing_file_full_ret;
>>>
>>>if (strcmp(backing_file, curr_bs->backing_file) == 0) {
>>
>> hmm, interesting, what bs->backing_file now means? It's strange enough to 
>> store such field on
>> bds, when we have backing link anyway..
> 
> Patch 37 has you covered. :-)
> 

Hmm, if it has removed this field, but it doesn't)

So, we finished with some object, called "overlay", but it is not an overlay of 
bs, it's overlay of
first non-implicit filtered node in bs backing chain, it may be found by 
bdrv_find_overlay() helper (which is
almost unused and my be safely dropped), and filename of this "overlay" is 
stored in bs->backing_file string
variable, keeping in mind that bs->backing is pointer to backing child of bs 
which is completely another thing?

Oh, no, everything related to filename-based backing chain logic is not for me 
o_O. If something doesn't work
with filename-based logic users should use node-names.. And I'd prefer to 
deprecate filename based interfaces
at all.

-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/13/19 9:31 PM, Max Reitz wrote:
> On 13.06.19 15:20, Pino Toscano wrote:
>> Rewrite the implementation of the ssh block driver to use libssh instead
>> of libssh2.  The libssh library has various advantages over libssh2:
>> - easier API for authentication (for example for using ssh-agent)
>> - easier API for known_hosts handling
>> - supports newer types of keys in known_hosts
>>
>> Use APIs/features available in libssh 0.8 conditionally, to support
>> older versions (which are not recommended though).
>>
>> Adjust the tests according to the different error message, and to the
>> newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
>> and libssh >= 0.7.0.
>>
>> Adjust the various Docker/Travis scripts to use libssh when available
>> instead of libssh2. The mingw/mxe testing is dropped for now, as there
>> are no packages for it.
>>
>> Signed-off-by: Pino Toscano 
>> Tested-by: Philippe Mathieu-Daudé 
>> Acked-by: Alex Bennée 
>> ---
> 
> Can confirm that this runs much faster than the last version I tested.
> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
> with me. :-)
> 
>> Changes from v8:
>> - use a newer key type in iotest 207
>> - improve the commit message
>>
>> Changes from v7:
>> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
>> - ptrdiff_t -> size_t
>>
>> Changes from v6:
>> - fixed few checkpatch style issues
>> - detect libssh 0.8 via symbol detection
>> - adjust travis/docker test material
>> - remove dead "default" case in a switch
>> - use variables for storing MIN() results
>> - adapt a documentation bit
>>
>> Changes from v5:
>> - adapt to newer tracing APIs
>> - disable ssh compression (mimic what libssh2 does by default)
>> - use build time checks for libssh 0.8, and use newer APIs directly
>>
>> Changes from v4:
>> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
>> - fix few return code checks
>> - remove now-unused parameters in few internal functions
>> - allow authentication with "none" method
>> - switch to unsigned int for the port number
>> - enable TCP_NODELAY on the socket
>> - fix one reference error message in iotest 207
>>
>> Changes from v3:
>> - fix socket cleanup in connect_to_ssh()
>> - add comments about the socket cleanup
>> - improve the error reporting (closer to what was with libssh2)
>> - improve EOF detection on sftp_read()
>>
>> Changes from v2:
>> - used again an own fd
>> - fixed co_yield() implementation
>>
>> Changes from v1:
>> - fixed jumbo packets writing
>> - fixed missing 'err' assignment
>> - fixed commit message
>>
>>  .travis.yml   |   4 +-
>>  block/Makefile.objs   |   6 +-
>>  block/ssh.c   | 622 +-
>>  block/trace-events|  14 +-
>>  configure |  65 +-
>>  docs/qemu-block-drivers.texi  |   2 +-
>>  .../dockerfiles/debian-win32-cross.docker |   1 -
>>  .../dockerfiles/debian-win64-cross.docker |   1 -
>>  tests/docker/dockerfiles/fedora.docker|   4 +-
>>  tests/docker/dockerfiles/ubuntu.docker|   2 +-
>>  tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
>>  tests/qemu-iotests/207|   4 +-
>>  tests/qemu-iotests/207.out|   2 +-
>>  13 files changed, 376 insertions(+), 353 deletions(-)
> 
> Surprisingly little has changed since v4.  Good, good, that makes my
> reviewing job much easier because I can thus rely on past me. :-)
> 
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 6da7b9cbfe..fb458d4548 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
> 
> [...]
> 
>> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename, 
>> QDict *options,
>>  parse_uri(filename, options, errp);
>>  }
>>  
>> -static int check_host_key_knownhosts(BDRVSSHState *s,
>> - const char *host, int port, Error 
>> **errp)
>> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
>>  {
> 
> [...]
> 
>> -switch (r) {
>> -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
>> +switch (state) {
>> +case SSH_KNOWN_HOSTS_OK:
>>  /* OK */
>> -trace_ssh_check_host_key_knownhosts(found->key);
>> +trace_ssh_check_host_key_knownhosts();
>>  break;
>> -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
>> +case SSH_KNOWN_HOSTS_CHANGED:
>>  ret = -EINVAL;
>> -session_error_setg(errp, s,
>> -  "host key does not match the one in known_hosts"
>> -  " (found key %s)", found->key);
>> +error_setg(errp, "host key does not match the one in known_hosts");
> 
> So what about the possible attack warning that the specification
> technically requires us to print? O:-)
> 
>>  goto out;
>> -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
>> +case SSH_KNOWN_HOSTS_OTHER:
>>  ret = -EINVAL;
>> -

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Pino Toscano]

On Thursday, 13 June 2019 21:31:40 CEST Max Reitz wrote:
> On 13.06.19 15:20, Pino Toscano wrote:
> > -switch (r) {
> > -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> > +switch (state) {
> > +case SSH_KNOWN_HOSTS_OK:
> >  /* OK */
> > -trace_ssh_check_host_key_knownhosts(found->key);
> > +trace_ssh_check_host_key_knownhosts();
> >  break;
> > -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> > +case SSH_KNOWN_HOSTS_CHANGED:
> >  ret = -EINVAL;
> > -session_error_setg(errp, s,
> > -  "host key does not match the one in known_hosts"
> > -  " (found key %s)", found->key);
> > +error_setg(errp, "host key does not match the one in known_hosts");
> 
> So what about the possible attack warning that the specification
> technically requires us to print? O:-)

There is the API since libssh 0.8.0... which unfortunately is not
usable, as they forgot to properly export the symbol :-(

> 
> >  goto out;
> > -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
> > +case SSH_KNOWN_HOSTS_OTHER:
> >  ret = -EINVAL;
> > -session_error_setg(errp, s, "no host key was found in 
> > known_hosts");
> > +error_setg(errp,
> > +   "host key for this server not found, another type 
> > exists");
> >  goto out;
> > -case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
> > +case SSH_KNOWN_HOSTS_UNKNOWN:
> >  ret = -EINVAL;
> > -session_error_setg(errp, s,
> > -  "failure matching the host key with known_hosts");
> > +error_setg(errp, "no host key was found in known_hosts");
> > +goto out;
> > +case SSH_KNOWN_HOSTS_NOT_FOUND:
> > +ret = -ENOENT;
> > +error_setg(errp, "known_hosts file not found");
> > +goto out;
> > +case SSH_KNOWN_HOSTS_ERROR:
> > +ret = -EINVAL;
> > +error_setg(errp, "error while checking the host");
> >  goto out;
> >  default:
> >  ret = -EINVAL;
> > -session_error_setg(errp, s, "unknown error matching the host key"
> > -  " with known_hosts (%d)", r);
> > +error_setg(errp, "error while checking for known server");
> >  goto out;
> >  }
> > +#else /* !HAVE_LIBSSH_0_8 */
> > +int state;
> > +
> > +state = ssh_is_server_known(s->session);
> > +trace_ssh_server_status(state);
> > +
> > +switch (state) {
> > +case SSH_SERVER_KNOWN_OK:
> > +/* OK */
> > +trace_ssh_check_host_key_knownhosts();
> > +break;
> > +case SSH_SERVER_KNOWN_CHANGED:
> > +ret = -EINVAL;
> > +error_setg(errp, "host key does not match the one in known_hosts");
> > +goto out;
> > +case SSH_SERVER_FOUND_OTHER:
> > +ret = -EINVAL;
> > +error_setg(errp,
> > +   "host key for this server not found, another type 
> > exists");
> > +goto out;
> > +case SSH_SERVER_FILE_NOT_FOUND:
> > +ret = -ENOENT;
> > +error_setg(errp, "known_hosts file not found");
> > +goto out;
> > +case SSH_SERVER_NOT_KNOWN:
> > +ret = -EINVAL;
> > +error_setg(errp, "no host key was found in known_hosts");
> > +goto out;
> > +case SSH_SERVER_ERROR:
> > +ret = -EINVAL;
> > +error_setg(errp, "server error");
> > +goto out;
> 
> No default here?

This switch is for libssh < 0.8.0, so enumerating all the possible
values of the enum of the old API is enough.

> > @@ -775,16 +845,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict 
> > *options, int bdrv_flags,
> >  }
> >  
> >  /* Go non-blocking. */
> > -libssh2_session_set_blocking(s->session, 0);
> > +ssh_set_blocking(s->session, 0);
> >  
> >  qapi_free_BlockdevOptionsSsh(opts);
> >  
> >  return 0;
> >  
> >   err:
> > -if (s->sock >= 0) {
> > -close(s->sock);
> > -}
> >  s->sock = -1;
> 
> Shouldn’t connect_to_ssh() set this to -1 after closing the session?

It should, although it will not make a difference. connect_to_ssh() is
used in only two places:
- in ssh_file_open, and s->sock is reset to -1 on error
- in ssh_co_create, which uses a local BDRVSSHState

> > diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
> > index b3816136f7..774eb5f2a9 100755
> > --- a/tests/qemu-iotests/207
> > +++ b/tests/qemu-iotests/207
> 
> [...]
> 
> > @@ -149,7 +149,7 @@ with iotests.FilePath('t.img') as disk_path, \
> >  iotests.img_info_log(remote_path)
> >  
> >  sha1_key = subprocess.check_output(
> > -'ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" | ' +
> > +'ssh-keyscan -t ssh-ed25519 127.0.0.1 2>/dev/null | grep -v "\\^#" 
> > | ' +
> >  'cut -d" " -f3 | base64 -d | sha1sum -b | cut -d" " -f1',
> >  shell=True).rstrip().decode('ascii')
> 
> Hm.  This is a pain.
> 
> I suppose the best would be to drop the "-t" altogether and 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/6] configure: Try to fix --static linking

[Peter Maydell]

On Fri, 14 Jun 2019 at 14:58, Alex Bennée  wrote:

> It would be nice to have a --static-user config flag and deprecate the
> --static flag. I don't think there is a decent use case for system
> emulation targets.

It would be really tricky to build half with static and half
without: our configure and build system really assumes that
fundamental stuff like "what libraries" and "what compiler flags"
are the same across the whole of the build.

Is --static-user really much better than:
 * allow --static --disable-system --disable-tools
 * forbid --static without --disable-system --disable-tools
 * require users to build the static usermode binaries separately
   from the system/tools build

(which is in practice what we have now) ?

Debian wants both static usermode and non-static usermode
binaries, so they'd still need to build multiple times anyway.

thanks
-- PMM

Re: [Qemu-block] [PATCH 6/6] .travis.yml: Test static linking

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> This job currently fails:
>
>   LINKlm32-softmmu/qemu-system-lm32
> /usr/bin/ld: cannot find -lgtk-3
> /usr/bin/ld: cannot find -latk-bridge-2.0
> /usr/bin/ld: cannot find -latspi
> /usr/bin/ld: cannot find -lsystemd
> /usr/bin/ld: cannot find -lgdk-3
> /usr/bin/ld: cannot find -lwayland-egl
> /usr/bin/ld: cannot find -lmirclient
> /usr/bin/ld: cannot find -lmircore
> /usr/bin/ld: cannot find -lmircookie
> /usr/bin/ld: cannot find -lepoxy
> /usr/bin/ld: cannot find -latk-1.0
> /usr/bin/ld: cannot find -lgdk_pixbuf-2.0
> /usr/bin/ld: cannot find -lselinux
> /usr/bin/ld: cannot find -lgtk-3
> /usr/bin/ld: cannot find -latk-bridge-2.0
> /usr/bin/ld: cannot find -latspi
> /usr/bin/ld: cannot find -lsystemd
> /usr/bin/ld: cannot find -lgdk-3
> /usr/bin/ld: cannot find -lwayland-egl
> /usr/bin/ld: cannot find -lmirclient
> /usr/bin/ld: cannot find -lmircore
> /usr/bin/ld: cannot find -lmircookie
> /usr/bin/ld: cannot find -lepoxy
> /usr/bin/ld: cannot find -latk-1.0
> /usr/bin/ld: cannot find -lgdk_pixbuf-2.0
> /usr/bin/ld: cannot find -lselinux
> /usr/bin/ld: attempted static link of dynamic object 
> `/usr/lib/x86_64-linux-gnu/libz.so'
> collect2: error: ld returned 1 exit status
> Makefile:204: recipe for target 'qemu-system-lm32' failed
> make[1]: *** [qemu-system-lm32] Error 1
> Makefile:472: recipe for target 'subdir-lm32-softmmu' failed
> make: *** [subdir-lm32-softmmu] Error 2
> ---
>  .travis.yml | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/.travis.yml b/.travis.yml
> index 08502c0aa2..6962fff826 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -92,6 +92,11 @@ matrix:
>  - CONFIG="--disable-user 
> --target-list-exclude=${MAIN_SOFTMMU_TARGETS}"
>
>
> +# Test static linking
> +- env:
> +- CONFIG="--static --target-list=lm32-softmmu"
> +
> +

It's probably more useful to have a:

  CONFIG="--disable-system --static"

In fact arguably we could just add it to the first --disable-system
stanza as there are other linux-user builds scattered about to catch the
cases where we break dynamically linked linux-user builds.

>  # Just build tools and run minimal unit and softfloat checks
>  - env:
>  - BASE_CONFIG="--enable-tools"


--
Alex Bennée

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Max Reitz]

On 14.06.19 16:29, Pino Toscano wrote:
> On Thursday, 13 June 2019 21:31:40 CEST Max Reitz wrote:
>> On 13.06.19 15:20, Pino Toscano wrote:
>>> -switch (r) {
>>> -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
>>> +switch (state) {
>>> +case SSH_KNOWN_HOSTS_OK:
>>>  /* OK */
>>> -trace_ssh_check_host_key_knownhosts(found->key);
>>> +trace_ssh_check_host_key_knownhosts();
>>>  break;
>>> -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
>>> +case SSH_KNOWN_HOSTS_CHANGED:
>>>  ret = -EINVAL;
>>> -session_error_setg(errp, s,
>>> -  "host key does not match the one in known_hosts"
>>> -  " (found key %s)", found->key);
>>> +error_setg(errp, "host key does not match the one in known_hosts");
>>
>> So what about the possible attack warning that the specification
>> technically requires us to print? O:-)
> 
> There is the API since libssh 0.8.0... which unfortunately is not
> usable, as they forgot to properly export the symbol :-(

Actuall, I just meant adding some wording about that to the error message.

>>>  goto out;
>>> -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
>>> +case SSH_KNOWN_HOSTS_OTHER:
>>>  ret = -EINVAL;
>>> -session_error_setg(errp, s, "no host key was found in 
>>> known_hosts");
>>> +error_setg(errp,
>>> +   "host key for this server not found, another type 
>>> exists");
>>>  goto out;
>>> -case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
>>> +case SSH_KNOWN_HOSTS_UNKNOWN:
>>>  ret = -EINVAL;
>>> -session_error_setg(errp, s,
>>> -  "failure matching the host key with known_hosts");
>>> +error_setg(errp, "no host key was found in known_hosts");
>>> +goto out;
>>> +case SSH_KNOWN_HOSTS_NOT_FOUND:
>>> +ret = -ENOENT;
>>> +error_setg(errp, "known_hosts file not found");
>>> +goto out;
>>> +case SSH_KNOWN_HOSTS_ERROR:
>>> +ret = -EINVAL;
>>> +error_setg(errp, "error while checking the host");
>>>  goto out;
>>>  default:
>>>  ret = -EINVAL;
>>> -session_error_setg(errp, s, "unknown error matching the host key"
>>> -  " with known_hosts (%d)", r);
>>> +error_setg(errp, "error while checking for known server");
>>>  goto out;
>>>  }
>>> +#else /* !HAVE_LIBSSH_0_8 */
>>> +int state;
>>> +
>>> +state = ssh_is_server_known(s->session);
>>> +trace_ssh_server_status(state);
>>> +
>>> +switch (state) {
>>> +case SSH_SERVER_KNOWN_OK:
>>> +/* OK */
>>> +trace_ssh_check_host_key_knownhosts();
>>> +break;
>>> +case SSH_SERVER_KNOWN_CHANGED:
>>> +ret = -EINVAL;
>>> +error_setg(errp, "host key does not match the one in known_hosts");
>>> +goto out;
>>> +case SSH_SERVER_FOUND_OTHER:
>>> +ret = -EINVAL;
>>> +error_setg(errp,
>>> +   "host key for this server not found, another type 
>>> exists");
>>> +goto out;
>>> +case SSH_SERVER_FILE_NOT_FOUND:
>>> +ret = -ENOENT;
>>> +error_setg(errp, "known_hosts file not found");
>>> +goto out;
>>> +case SSH_SERVER_NOT_KNOWN:
>>> +ret = -EINVAL;
>>> +error_setg(errp, "no host key was found in known_hosts");
>>> +goto out;
>>> +case SSH_SERVER_ERROR:
>>> +ret = -EINVAL;
>>> +error_setg(errp, "server error");
>>> +goto out;
>>
>> No default here?
> 
> This switch is for libssh < 0.8.0, so enumerating all the possible
> values of the enum of the old API is enough.

state is an integer.  I feel very uneasy about not having a default
clause for a plain integer, especially if it is supplied by an external
library.

>>> @@ -775,16 +845,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict 
>>> *options, int bdrv_flags,
>>>  }
>>>  
>>>  /* Go non-blocking. */
>>> -libssh2_session_set_blocking(s->session, 0);
>>> +ssh_set_blocking(s->session, 0);
>>>  
>>>  qapi_free_BlockdevOptionsSsh(opts);
>>>  
>>>  return 0;
>>>  
>>>   err:
>>> -if (s->sock >= 0) {
>>> -close(s->sock);
>>> -}
>>>  s->sock = -1;
>>
>> Shouldn’t connect_to_ssh() set this to -1 after closing the session?
> 
> It should, although it will not make a difference. connect_to_ssh() is
> used in only two places:
> - in ssh_file_open, and s->sock is reset to -1 on error
> - in ssh_co_create, which uses a local BDRVSSHState

I meant: Why don’t you move this to connect_to_ssh()?

>>> diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
>>> index b3816136f7..774eb5f2a9 100755
>>> --- a/tests/qemu-iotests/207
>>> +++ b/tests/qemu-iotests/207
>>
>> [...]
>>
>>> @@ -149,7 +149,7 @@ with iotests.FilePath('t.img') as disk_path, \
>>>  iotests.img_info_log(remote_path)
>>>  
>>>  sha1_key = subprocess.check_output(
>>> -'ssh-keyscan -t rsa 

Re: [Qemu-block] [PATCH v9] ssh: switch from libssh2 to libssh

[Max Reitz]

On 14.06.19 16:26, Philippe Mathieu-Daudé wrote:
> On 6/13/19 9:31 PM, Max Reitz wrote:
>> On 13.06.19 15:20, Pino Toscano wrote:
>>> Rewrite the implementation of the ssh block driver to use libssh instead
>>> of libssh2.  The libssh library has various advantages over libssh2:
>>> - easier API for authentication (for example for using ssh-agent)
>>> - easier API for known_hosts handling
>>> - supports newer types of keys in known_hosts
>>>
>>> Use APIs/features available in libssh 0.8 conditionally, to support
>>> older versions (which are not recommended though).
>>>
>>> Adjust the tests according to the different error message, and to the
>>> newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
>>> and libssh >= 0.7.0.
>>>
>>> Adjust the various Docker/Travis scripts to use libssh when available
>>> instead of libssh2. The mingw/mxe testing is dropped for now, as there
>>> are no packages for it.
>>>
>>> Signed-off-by: Pino Toscano 
>>> Tested-by: Philippe Mathieu-Daudé 
>>> Acked-by: Alex Bennée 
>>> ---
>>
>> Can confirm that this runs much faster than the last version I tested.
>> 197 and 215 are still whacky (like 100 s instead of 50), but that’s fine
>> with me. :-)
>>
>>> Changes from v8:
>>> - use a newer key type in iotest 207
>>> - improve the commit message
>>>
>>> Changes from v7:
>>> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
>>> - ptrdiff_t -> size_t
>>>
>>> Changes from v6:
>>> - fixed few checkpatch style issues
>>> - detect libssh 0.8 via symbol detection
>>> - adjust travis/docker test material
>>> - remove dead "default" case in a switch
>>> - use variables for storing MIN() results
>>> - adapt a documentation bit
>>>
>>> Changes from v5:
>>> - adapt to newer tracing APIs
>>> - disable ssh compression (mimic what libssh2 does by default)
>>> - use build time checks for libssh 0.8, and use newer APIs directly
>>>
>>> Changes from v4:
>>> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
>>> - fix few return code checks
>>> - remove now-unused parameters in few internal functions
>>> - allow authentication with "none" method
>>> - switch to unsigned int for the port number
>>> - enable TCP_NODELAY on the socket
>>> - fix one reference error message in iotest 207
>>>
>>> Changes from v3:
>>> - fix socket cleanup in connect_to_ssh()
>>> - add comments about the socket cleanup
>>> - improve the error reporting (closer to what was with libssh2)
>>> - improve EOF detection on sftp_read()
>>>
>>> Changes from v2:
>>> - used again an own fd
>>> - fixed co_yield() implementation
>>>
>>> Changes from v1:
>>> - fixed jumbo packets writing
>>> - fixed missing 'err' assignment
>>> - fixed commit message
>>>
>>>  .travis.yml   |   4 +-
>>>  block/Makefile.objs   |   6 +-
>>>  block/ssh.c   | 622 +-
>>>  block/trace-events|  14 +-
>>>  configure |  65 +-
>>>  docs/qemu-block-drivers.texi  |   2 +-
>>>  .../dockerfiles/debian-win32-cross.docker |   1 -
>>>  .../dockerfiles/debian-win64-cross.docker |   1 -
>>>  tests/docker/dockerfiles/fedora.docker|   4 +-
>>>  tests/docker/dockerfiles/ubuntu.docker|   2 +-
>>>  tests/docker/dockerfiles/ubuntu1804.docker|   2 +-
>>>  tests/qemu-iotests/207|   4 +-
>>>  tests/qemu-iotests/207.out|   2 +-
>>>  13 files changed, 376 insertions(+), 353 deletions(-)
>>
>> Surprisingly little has changed since v4.  Good, good, that makes my
>> reviewing job much easier because I can thus rely on past me. :-)
>>
>>> diff --git a/block/ssh.c b/block/ssh.c
>>> index 6da7b9cbfe..fb458d4548 100644
>>> --- a/block/ssh.c
>>> +++ b/block/ssh.c
>>
>> [...]
>>
>>> @@ -282,82 +274,85 @@ static void ssh_parse_filename(const char *filename, 
>>> QDict *options,
>>>  parse_uri(filename, options, errp);
>>>  }
>>>  
>>> -static int check_host_key_knownhosts(BDRVSSHState *s,
>>> - const char *host, int port, Error 
>>> **errp)
>>> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
>>>  {
>>
>> [...]
>>
>>> -switch (r) {
>>> -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
>>> +switch (state) {
>>> +case SSH_KNOWN_HOSTS_OK:
>>>  /* OK */
>>> -trace_ssh_check_host_key_knownhosts(found->key);
>>> +trace_ssh_check_host_key_knownhosts();
>>>  break;
>>> -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
>>> +case SSH_KNOWN_HOSTS_CHANGED:
>>>  ret = -EINVAL;
>>> -session_error_setg(errp, s,
>>> -  "host key does not match the one in known_hosts"
>>> -  " (found key %s)", found->key);
>>> +error_setg(errp, "host key does not match the one in known_hosts");
>>
>> So what about the possible attack warning that the specification
>> technically requires 

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Pino Toscano]

On Thursday, 13 June 2019 21:41:58 CEST Stefan Weil wrote:
> On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
> > Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
> [...]
> > Note, libssh is not available on MinGW.
> 
> Nor is it available for Mingw64:
> 
> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh=x86_64
> 
> That makes building for Windows more difficult because there is an
> additional dependency which must be built from source.

Yes, sorry for that.  However this can be fixed easily by creating
Mingw packages of libssh (not volunteering myself, sorry).

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

[Qemu-block] [PULL 17/20] blkdebug: Add "none" event

[Max Reitz]

Together with @iotypes and @sector, this can be used to trap e.g. the
first read or write access to a certain sector without having to know
what happens internally in the block layer, i.e. which "real" events
happen right before such an access.

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190507203508.18026-5-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 4 +++-
 block/blkdebug.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 34617a2c7a..60f903afa3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3244,6 +3244,8 @@
 #
 # @cluster_alloc_space: an allocation of file space for a cluster (since 4.1)
 #
+# @none: triggers once at creation of the blkdebug node (since 4.1)
+#
 # Since: 2.9
 ##
 { 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
@@ -3262,7 +3264,7 @@
 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
-'cor_write', 'cluster_alloc_space'] }
+'cor_write', 'cluster_alloc_space', 'none'] }
 
 ##
 # @BlkdebugIOType:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 3f3ec11230..1663ed25af 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -491,6 +491,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto out;
 }
 
+bdrv_debug_event(bs, BLKDBG_NONE);
+
 ret = 0;
 out:
 if (ret < 0) {
-- 
2.21.0

[Qemu-block] [PULL 19/20] iotests: Test qemu-img convert --salvage

[Max Reitz]

This test converts a simple image to another, but blkdebug injects
block_status and read faults at some offsets.  The resulting image
should be the same as the input image, except that sectors that could
not be read have to be 0.

Signed-off-by: Max Reitz 
Message-id: 20190507203508.18026-7-mre...@redhat.com
Tested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
[mreitz: Dropped superfluous printf from _filter_offsets, as suggested
 by Vladimir; disable test for VDI and IMGOPTSSYNTAX]
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/251 | 170 +
 tests/qemu-iotests/251.out |  43 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 214 insertions(+)
 create mode 100755 tests/qemu-iotests/251
 create mode 100644 tests/qemu-iotests/251.out

diff --git a/tests/qemu-iotests/251 b/tests/qemu-iotests/251
new file mode 100755
index 00..13f85de9cd
--- /dev/null
+++ b/tests/qemu-iotests/251
@@ -0,0 +1,170 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img convert --salvage
+#
+# Copyright (C) 2019 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=mre...@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+if [ "$IMGOPTSSYNTAX" = "true" ]; then
+# We use json:{} filenames here, so we cannot work with additional options.
+_unsupported_fmt $IMGFMT
+else
+# With VDI, the output is ordered differently.  Just disable it.
+_unsupported_fmt vdi
+fi
+
+
+TEST_IMG="$TEST_IMG.orig" _make_test_img 64M
+
+$QEMU_IO -c 'write -P 42 0 64M' "$TEST_IMG.orig" | _filter_qemu_io
+
+
+sector_size=512
+
+# Offsets on which to fail block-status.  Keep in ascending order so
+# the indexing done by _filter_offsets will appear in ascending order
+# in the output as well.
+status_fail_offsets="$((16 * 1024 * 1024 + 8192))
+ $((33 * 1024 * 1024 + 512))"
+
+# Offsets on which to fail reads.  Keep in ascending order for the
+# same reason.
+# The second element is shared with $status_fail_offsets on purpose.
+# Starting with the third element, we test what happens when a
+# continuous range of sectors is inaccessible.
+read_fail_offsets="$((32 * 1024 * 1024 - 65536))
+   $((33 * 1024 * 1024 + 512))
+   $(seq $((34 * 1024 * 1024)) $sector_size \
+ $((34 * 1024 * 1024 + 4096 - $sector_size)))"
+
+
+# blkdebug must be above the format layer so it can intercept all
+# block-status events
+source_img="json:{'driver': 'blkdebug',
+  'image': {
+  'driver': '$IMGFMT',
+  'file': {
+  'driver': 'file',
+  'filename': '$TEST_IMG.orig'
+  }
+  },
+  'inject-error': ["
+
+for ofs in $status_fail_offsets
+do
+source_img+="{ 'event': 'none',
+   'iotype': 'block-status',
+   'errno': 5,
+   'sector': $((ofs / sector_size)) },"
+done
+
+for ofs in $read_fail_offsets
+do
+source_img+="{ 'event': 'none',
+   'iotype': 'read',
+   'errno': 5,
+   'sector': $((ofs / sector_size)) },"
+done
+
+# Remove the trailing comma and terminate @inject-error and json:{}
+source_img="${source_img%,} ] }"
+
+
+echo
+
+
+_filter_offsets() {
+filters=
+
+index=0
+for ofs in $1
+do
+filters+=" -e s/$ofs/status_fail_offset_$index/"
+index=$((index + 1))
+done
+
+index=0
+for ofs in $2
+do
+filters+=" -e s/$ofs/read_fail_offset_$index/"
+index=$((index + 1))
+done
+
+sed $filters
+}
+
+# While determining the number of allocated sectors in the input
+# image, we should see one block status warning per element of
+# $status_fail_offsets.
+#
+# Then, the image is read.  Since the block status is queried in
+# basically the same way, the same warnings as in the previous step
+# should reappear.  Interleaved with those we should see a read
+# 

[Qemu-block] [PULL 15/20] qemu-img: Add salvaging mode to convert

[Max Reitz]

This adds a salvaging mode (--salvage) to qemu-img convert which ignores
read errors and treats the respective areas as containing only zeroes.
This can be used for instance to at least partially recover the data
from terminally corrupted qcow2 images.

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190507203508.18026-3-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 qemu-img.c   | 90 +---
 qemu-img-cmds.hx |  4 +--
 qemu-img.texi|  4 +++
 3 files changed, 75 insertions(+), 23 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e15e617256..158b3a505f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -69,6 +69,7 @@ enum {
 OPTION_SIZE = 264,
 OPTION_PREALLOCATION = 265,
 OPTION_SHRINK = 266,
+OPTION_SALVAGE = 267,
 };
 
 typedef enum OutputFormat {
@@ -1581,6 +1582,7 @@ typedef struct ImgConvertState {
 int64_t target_backing_sectors; /* negative if unknown */
 bool wr_in_order;
 bool copy_range;
+bool salvage;
 bool quiet;
 int min_sparse;
 int alignment;
@@ -1628,25 +1630,44 @@ static int convert_iteration_sectors(ImgConvertState 
*s, int64_t sector_num)
 }
 
 if (s->sector_next_status <= sector_num) {
-int64_t count = n * BDRV_SECTOR_SIZE;
+uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE;
+int64_t count;
 
-if (s->target_has_backing) {
+do {
+count = n * BDRV_SECTOR_SIZE;
+
+if (s->target_has_backing) {
+ret = bdrv_block_status(blk_bs(s->src[src_cur]), offset,
+count, , NULL, NULL);
+} else {
+ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
+  offset, count, , NULL,
+  NULL);
+}
+
+if (ret < 0) {
+if (s->salvage) {
+if (n == 1) {
+if (!s->quiet) {
+warn_report("error while reading block status at "
+"offset %" PRIu64 ": %s", offset,
+strerror(-ret));
+}
+/* Just try to read the data, then */
+ret = BDRV_BLOCK_DATA;
+count = BDRV_SECTOR_SIZE;
+} else {
+/* Retry on a shorter range */
+n = DIV_ROUND_UP(n, 4);
+}
+} else {
+error_report("error while reading block status at offset "
+ "%" PRIu64 ": %s", offset, strerror(-ret));
+return ret;
+}
+}
+} while (ret < 0);
 
-ret = bdrv_block_status(blk_bs(s->src[src_cur]),
-(sector_num - src_cur_offset) *
-BDRV_SECTOR_SIZE,
-count, , NULL, NULL);
-} else {
-ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
-  (sector_num - src_cur_offset) *
-  BDRV_SECTOR_SIZE,
-  count, , NULL, NULL);
-}
-if (ret < 0) {
-error_report("error while reading block status of sector %" PRId64
- ": %s", sector_num, strerror(-ret));
-return ret;
-}
 n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
 
 if (ret & BDRV_BLOCK_ZERO) {
@@ -1683,6 +1704,7 @@ static int convert_iteration_sectors(ImgConvertState *s, 
int64_t sector_num)
 static int coroutine_fn convert_co_read(ImgConvertState *s, int64_t sector_num,
 int nb_sectors, uint8_t *buf)
 {
+uint64_t single_read_until = 0;
 int n, ret;
 
 assert(nb_sectors <= s->buf_sectors);
@@ -1690,6 +1712,7 @@ static int coroutine_fn convert_co_read(ImgConvertState 
*s, int64_t sector_num,
 BlockBackend *blk;
 int src_cur;
 int64_t bs_sectors, src_cur_offset;
+uint64_t offset;
 
 /* In the case of compression with multiple source files, we can get a
  * nb_sectors that spreads into the next part. So we must be able to
@@ -1698,13 +1721,29 @@ static int coroutine_fn convert_co_read(ImgConvertState 
*s, int64_t sector_num,
 blk = s->src[src_cur];
 bs_sectors = s->src_sectors[src_cur];
 
+offset = (sector_num - src_cur_offset) << BDRV_SECTOR_BITS;
+
 n = MIN(nb_sectors, bs_sectors - (sector_num - src_cur_offset));
+if (single_read_until > offset) {
+n = 1;
+}
 
-ret = blk_co_pread(
-blk, (sector_num - src_cur_offset) << 

Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Max Reitz]

On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Use child access functions when iterating through backing chains so
>> filters do not break the chain.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block.c | 40 
>>   1 file changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 11f37983d9..505b3e9a01 100644
>> --- a/block.c
>> +++ b/block.c

[...]

>> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>   BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>>   BlockDriverState *bs)
>>   {
>> -while (active && bs != backing_bs(active)) {
>> -active = backing_bs(active);
>> +bs = bdrv_skip_rw_filters(bs);
>> +active = bdrv_skip_rw_filters(active);
>> +
>> +while (active) {
>> +BlockDriverState *next = bdrv_backing_chain_next(active);
>> +if (bs == next) {
>> +return active;
>> +}
>> +active = next;
>>   }
>>   
>> -return active;
>> +return NULL;
>>   }
> 
> Semantics changed for this function.
> It is used in two places
> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we 
> will never have
> filter node as a bottom of some valid chain
> 
> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't 
> understand,
> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs 
> overlay is out of the job,
> what is this check for?

There is a loop before this check which checks that the same blocker is
not set on any nodes between top and base (both inclusive).  I guess
non-active commit checks the node above @top, too, because its backing
file will change.

>>   /* Given a BDS, searches for the base layer. */

[...]

>> @@ -5149,7 +5165,7 @@ BlockDriverState 
>> *bdrv_find_backing_image(BlockDriverState *bs,
>>   char *backing_file_full_ret;
>>   
>>   if (strcmp(backing_file, curr_bs->backing_file) == 0) {
> 
> hmm, interesting, what bs->backing_file now means? It's strange enough to 
> store such field on
> bds, when we have backing link anyway..

Patch 37 has you covered. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 0/8] block: Ignore loosening perm restrictions failures

[Kevin Wolf]

Am 22.05.2019 um 19:03 hat Max Reitz geschrieben:
> Hi,
> 
> This series is mainly a fix for
> https://bugzilla.redhat.com/show_bug.cgi?id=1703793.  The problem
> described there is that mirroring to a gluster volume, then switching
> off the volume makes qemu crash.  There are two problems here:
> 
> (1) file-posix reopens the FD all the time because it thinks the FD it
> has is RDONLY.  It actually isn’t after the first reopen, we just
> forgot to change the internal flags.  That’s what patch 1 is for.
> 
> (2) Even then, when mirror completes, it drops its write permission on
> the FD.  This requires a reopen, which will fail if the volume is
> down.  Mirror doesn’t expect that.  Nobody ever expects that
> dropping permissions can fail, and rightfully so because that’s what
> I think we have generally agreed on.
> Therefore, the block layer should hide this error.  This is what the
> last two patches are for.
> 
> The penultimate patch adds two assertions: bdrv_replace_child() (for the
> old BDS) and bdrv_inactivate_recurse() assume they only ever drop
> assertions.  This is now substantiated by these new assertions.
> It turns out that this assumption was just plain wrong.  Patches 3 to 5
> make it right.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v5 16/42] block: Use child access functions when flushing

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> If the driver does not support .bdrv_co_flush() so bdrv_co_flush()
> itself has to flush the children of the given node, it should not flush
> just bs->file->bs, but in fact both the child that stores data, and the
> one that stores metadata (if they are separate).
> 
> In any case, the BLKDBG_EVENT() should be emitted on the primary child,
> because that is where a blkdebug node would be if there is any.
> 
> Signed-off-by: Max Reitz 
> ---
>   block/io.c | 21 ++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 53aabf86b5..64408cf19a 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2533,6 +2533,8 @@ static void coroutine_fn bdrv_flush_co_entry(void 
> *opaque)
>   
>   int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>   {
> +BdrvChild *primary_child = bdrv_primary_child(bs);
> +BlockDriverState *storage_bs, *metadata_bs;
>   int current_gen;
>   int ret = 0;
>   
> @@ -2562,7 +2564,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>   }
>   
>   /* Write back cached data to the OS even with cache=unsafe */
> -BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS);
> +BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_OS);
>   if (bs->drv->bdrv_co_flush_to_os) {
>   ret = bs->drv->bdrv_co_flush_to_os(bs);
>   if (ret < 0) {
> @@ -2580,7 +2582,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>   goto flush_parent;
>   }
>   
> -BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
> +BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_DISK);
>   if (!bs->drv) {
>   /* bs->drv->bdrv_co_flush() might have ejected the BDS
>* (even in case of apparent success) */
> @@ -2625,7 +2627,20 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>* in the case of cache=unsafe, so there are no useless flushes.
>*/
>   flush_parent:
> -ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
> +storage_bs = bdrv_storage_bs(bs);
> +metadata_bs = bdrv_metadata_bs(bs);
> +
> +ret = 0;
> +if (storage_bs) {
> +ret = bdrv_co_flush(storage_bs);
> +}
> +if (metadata_bs && metadata_bs != storage_bs) {
> +int ret_metadata = bdrv_co_flush(metadata_bs);
> +if (!ret) {
> +ret = ret_metadata;
> +}
> +}
> +
>   out:
>   /* Notify any pending flushes that we have completed */
>   if (ret == 0) {
> 

Hmm, I'm not sure that if in one driver we decided to store data and metadata 
separately,
we need to support flushing them both generic code.. If at some point qcow2 
decides store part
of metadata in third child, we will not flush it here too?

Should not we instead loop through children and flush all? And I'd 
s/flush_parent/flush_children as
it is rather weird.

-- 
Best regards,
Vladimir

[Qemu-block] [PULL 13/20] blockdev: Overlays are not snapshots

[Max Reitz]

There are error messages which refer to an overlay node as the snapshot.
That is wrong, those are two different things.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Message-id: 20190603202236.1342-3-mre...@redhat.com
Reviewed-by: John Snow 
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 blockdev.c | 10 +-
 tests/qemu-iotests/085.out | 10 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fdafa173cc..b5c0fd3c49 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1608,13 +1608,13 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
 
 if (node_name && !snapshot_node_name) {
-error_setg(errp, "New snapshot node name missing");
+error_setg(errp, "New overlay node name missing");
 goto out;
 }
 
 if (snapshot_node_name &&
 bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-error_setg(errp, "New snapshot node name already in use");
+error_setg(errp, "New overlay node name already in use");
 goto out;
 }
 
@@ -1656,7 +1656,7 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 }
 
 if (bdrv_has_blk(state->new_bs)) {
-error_setg(errp, "The snapshot is already in use");
+error_setg(errp, "The overlay is already in use");
 goto out;
 }
 
@@ -1666,12 +1666,12 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 }
 
 if (state->new_bs->backing != NULL) {
-error_setg(errp, "The snapshot already has a backing image");
+error_setg(errp, "The overlay already has a backing image");
 goto out;
 }
 
 if (!state->new_bs->drv->supports_backing) {
-error_setg(errp, "The snapshot does not support backing images");
+error_setg(errp, "The overlay does not support backing images");
 goto out;
 }
 
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 6edf107f55..2a5f256cd3 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -64,13 +64,13 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/
 
 === Invalid command - cannot create a snapshot using a file BDS ===
 
-{"error": {"class": "GenericError", "desc": "The snapshot does not support 
backing images"}}
+{"error": {"class": "GenericError", "desc": "The overlay does not support 
backing images"}}
 
 === Invalid command - snapshot node used as active layer ===
 
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}
+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}
+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}
 
 === Invalid command - snapshot node used as backing hd ===
 
@@ -81,7 +81,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/t.IMGFMT.base
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "The snapshot already has a 
backing image"}}
+{"error": {"class": "GenericError", "desc": "The overlay already has a backing 
image"}}
 
 === Invalid command - The node does not exist ===
 
-- 
2.21.0

Re: [Qemu-block] [Qemu-devel] [PATCH 0/6] configure: Try to fix --static linking

[Alex Bennée]

Peter Maydell  writes:

> On Fri, 14 Jun 2019 at 08:27, Philippe Mathieu-Daudé  
> wrote:
>> Apparently QEMU static linking is slowly bitroting. Obviously it
>> depends the libraries an user has installed, anyway it seems there
>> are not much testing done.
>
> The main reason for supporting static linking is so we can build
> the user-mode emulators. Almost always the problems with
> static linking the softmmu binaries and the tools are
> issues with the distro's packaging of the static libraries
> (pkg-config files which specify things that don't work for
> static is a common one).
>
> So we could put in a lot of checking of "is what pkg-config
> tells us broken". Or we could just say "we don't support static
> linking for anything except the usermode binaries". We
> should probably phase in deprecation of that because it's
> possible somebody's using it seriously, but it seems like
> a fairly weird thing to do to me.

It would be nice to have a --static-user config flag and deprecate the
--static flag. I don't think there is a decent use case for system
emulation targets.

The Gentoo ebuild currently jumps through hoops to build QEMU by doing
the build twice, first for softmmu targets and then for user targets. I
suspect all of that is mostly to handle the reasonable "static-user" use
case which is what people really want*.

*I'm guessing

--
Alex Bennée

[Qemu-block] [PULL 16/20] blkdebug: Add @iotype error option

[Max Reitz]

This new error option allows users of blkdebug to inject errors only on
certain kinds of I/O operations.  Users usually want to make a very
specific operation fail, not just any; but right now they simply hope
that the event that triggers the error injection is followed up with
that very operation.  That may not be true, however, because the block
layer is changing (including blkdebug, which may increase the number of
types of I/O operations on which to inject errors).

The new option's default has been chosen to keep backwards
compatibility.

Note that similar to the internal representation, we could choose to
expose this option as a list of I/O types.  But there is no practical
use for this, because as described above, users usually know exactly
which kind of operation they want to make fail, so there is no need to
specify multiple I/O types at once.  In addition, exposing this option
as a list would require non-trivial changes to qemu_opts_absorb_qdict().

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190507203508.18026-4-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 26 +++
 block/blkdebug.c | 50 
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c0ff3a83ef..34617a2c7a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3264,6 +3264,26 @@
 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
 'cor_write', 'cluster_alloc_space'] }
 
+##
+# @BlkdebugIOType:
+#
+# Kinds of I/O that blkdebug can inject errors in.
+#
+# @read: .bdrv_co_preadv()
+#
+# @write: .bdrv_co_pwritev()
+#
+# @write-zeroes: .bdrv_co_pwrite_zeroes()
+#
+# @discard: .bdrv_co_pdiscard()
+#
+# @flush: .bdrv_co_flush_to_disk()
+#
+# Since: 4.1
+##
+{ 'enum': 'BlkdebugIOType', 'prefix': 'BLKDEBUG_IO_TYPE',
+  'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush' ] }
+
 ##
 # @BlkdebugInjectErrorOptions:
 #
@@ -3274,6 +3294,11 @@
 # @state:   the state identifier blkdebug needs to be in to
 #   actually trigger the event; defaults to "any"
 #
+# @iotype:  the type of I/O operations on which this error should
+#   be injected; defaults to "all read, write,
+#   write-zeroes, discard, and flush operations"
+#   (since: 4.1)
+#
 # @errno:   error identifier (errno) to be returned; defaults to
 #   EIO
 #
@@ -3291,6 +3316,7 @@
 { 'struct': 'BlkdebugInjectErrorOptions',
   'data': { 'event': 'BlkdebugEvent',
 '*state': 'int',
+'*iotype': 'BlkdebugIOType',
 '*errno': 'int',
 '*sector': 'int',
 '*once': 'bool',
diff --git a/block/blkdebug.c b/block/blkdebug.c
index efd9441625..3f3ec11230 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -75,6 +75,7 @@ typedef struct BlkdebugRule {
 int state;
 union {
 struct {
+uint64_t iotype_mask;
 int error;
 int immediately;
 int once;
@@ -91,6 +92,9 @@ typedef struct BlkdebugRule {
 QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
 } BlkdebugRule;
 
+QEMU_BUILD_BUG_MSG(BLKDEBUG_IO_TYPE__MAX > 64,
+   "BlkdebugIOType mask does not fit into an uint64_t");
+
 static QemuOptsList inject_error_opts = {
 .name = "inject-error",
 .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head),
@@ -103,6 +107,10 @@ static QemuOptsList inject_error_opts = {
 .name = "state",
 .type = QEMU_OPT_NUMBER,
 },
+{
+.name = "iotype",
+.type = QEMU_OPT_STRING,
+},
 {
 .name = "errno",
 .type = QEMU_OPT_NUMBER,
@@ -162,6 +170,8 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 int event;
 struct BlkdebugRule *rule;
 int64_t sector;
+BlkdebugIOType iotype;
+Error *local_error = NULL;
 
 /* Find the right event for the rule */
 event_name = qemu_opt_get(opts, "event");
@@ -192,6 +202,26 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 sector = qemu_opt_get_number(opts, "sector", -1);
 rule->options.inject.offset =
 sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
+
+iotype = qapi_enum_parse(_lookup,
+ qemu_opt_get(opts, "iotype"),
+ BLKDEBUG_IO_TYPE__MAX, _error);
+if (local_error) {
+error_propagate(errp, local_error);
+return -1;
+}
+if (iotype != BLKDEBUG_IO_TYPE__MAX) {
+rule->options.inject.iotype_mask = (1ull << iotype);
+} else {
+/* Apply the default */
+rule->options.inject.iotype_mask =
+(1ull << BLKDEBUG_IO_TYPE_READ)
+| (1ull << BLKDEBUG_IO_TYPE_WRITE)
+| (1ull << 

[Qemu-block] [PULL 10/20] iotests: restrict 254 to support only qcow2

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

Test fails at least for qcow, because of different cluster sizes in
base and top (and therefore different granularities of bitmaps we are
trying to merge).

The test aim is to check block-dirty-bitmap-merge between different
nodes functionality, no needs to check all formats. So, let's just drop
support for anything except qcow2.

Reported-by: Max Reitz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190605155405.104384-1-vsement...@virtuozzo.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/254 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index 33cb80a512..8edba91c5d 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -21,6 +21,8 @@
 import iotests
 from iotests import qemu_img_create, file_path, log
 
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
 disk, top = file_path('disk', 'top')
 size = 1024 * 1024
 
-- 
2.21.0

[Qemu-block] [PULL 07/20] iotests: Filter 175's allocation information

[Max Reitz]

It is possible for an empty file to take up blocks on a filesystem, for
example:

$ qemu-img create -f raw test.img 1G
Formatting 'test.img', fmt=raw size=1073741824
$ mkfs.ext4 -I 128 -q test.img
$ mkdir test-mount
$ sudo mount -o loop test.img test-mount
$ sudo touch test-mount/test-file
$ stat -c 'blocks=%b' test-mount/test-file
blocks=8

These extra blocks (one cluster) are apparently used for metadata,
because they are always there, on top of blocks used for data:

$ sudo dd if=/dev/zero of=test-mount/test-file bs=1M count=1
1+0 records in
1+0 records out
1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.00135339 s, 775 MB/s
$ stat -c 'blocks=%b' test-mount/test-file
blocks=2056

Make iotest 175 take this into account.

Reported-by: Thomas Huth 
Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Nir Soffer 
Message-id: 20190516144319.12570-1-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/175 | 26 ++
 tests/qemu-iotests/175.out |  8 
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
index d0ffc495c2..51e62c8276 100755
--- a/tests/qemu-iotests/175
+++ b/tests/qemu-iotests/175
@@ -28,10 +28,25 @@ status=1# failure is the default!
 
 _cleanup()
 {
-   _cleanup_test_img
+_cleanup_test_img
+rm -f "$TEST_DIR/empty"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
+# Some file systems sometimes allocate extra blocks independently of
+# the file size.  This function hides the resulting difference in the
+# stat -c '%b' output.
+# Parameter 1: Number of blocks an empty file occupies
+# Parameter 2: Image size in bytes
+_filter_blocks()
+{
+extra_blocks=$1
+img_size=$2
+
+sed -e "s/blocks=$extra_blocks\\(\$\\|[^0-9]\\)/nothing allocated/" \
+-e "s/blocks=$((extra_blocks + img_size / 
512))\\(\$\\|[^0-9]\\)/everything allocated/"
+}
+
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
@@ -40,18 +55,21 @@ _supported_fmt raw
 _supported_proto file
 _supported_os Linux
 
-size=1m
+size=$((1 * 1024 * 1024))
+
+touch "$TEST_DIR/empty"
+extra_blocks=$(stat -c '%b' "$TEST_DIR/empty")
 
 echo
 echo "== creating image with default preallocation =="
 _make_test_img $size | _filter_imgfmt
-stat -c "size=%s, blocks=%b" $TEST_IMG
+stat -c "size=%s, blocks=%b" $TEST_IMG | _filter_blocks $extra_blocks $size
 
 for mode in off full falloc; do
 echo
 echo "== creating image with preallocation $mode =="
 IMGOPTS=preallocation=$mode _make_test_img $size | _filter_imgfmt
-stat -c "size=%s, blocks=%b" $TEST_IMG
+stat -c "size=%s, blocks=%b" $TEST_IMG | _filter_blocks $extra_blocks $size
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
index 76c02c6a57..6d9a5ed84e 100644
--- a/tests/qemu-iotests/175.out
+++ b/tests/qemu-iotests/175.out
@@ -2,17 +2,17 @@ QA output created by 175
 
 == creating image with default preallocation ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
-size=1048576, blocks=0
+size=1048576, nothing allocated
 
 == creating image with preallocation off ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=off
-size=1048576, blocks=0
+size=1048576, nothing allocated
 
 == creating image with preallocation full ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=full
-size=1048576, blocks=2048
+size=1048576, everything allocated
 
 == creating image with preallocation falloc ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 preallocation=falloc
-size=1048576, blocks=2048
+size=1048576, everything allocated
  *** done
-- 
2.21.0

[Qemu-block] [PATCH] block/replication.c: Fix crash issue after failover

[Zhang Chen]

From: Zhang Chen 

No block job on active disk after failover.
In the replication_stop() function have canceled the block job,
we check it again here.

Signed-off-by: Zhang Chen 
---
 block/replication.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/replication.c b/block/replication.c
index 3d4dedddfc..bdf2bf4bbc 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -146,7 +146,9 @@ static void replication_close(BlockDriverState *bs)
 replication_stop(s->rs, false, NULL);
 }
 if (s->stage == BLOCK_REPLICATION_FAILOVER) {
-job_cancel_sync(>active_disk->bs->job->job);
+if (s->secondary_disk->bs->job) {
+job_cancel_sync(>secondary_disk->bs->job->job);
+}
 }
 
 if (s->mode == REPLICATION_MODE_SECONDARY) {
-- 
2.17.GIT

[Qemu-block] [PULL 09/20] hw/block/fdc: floppy command FIFO memory initialization

[Max Reitz]

From: Andrey Shinkevich 

The uninitialized memory allocated for the command FIFO of the
floppy controller during the VM hardware initialization incurs
many unwanted reports by Valgrind when VM state is being saved.
That verbosity hardens a search for the real memory issues when
the iotests run. Particularly, the patch eliminates 20 unnecessary
reports of the Valgrind tool in the iotest #169.

Signed-off-by: Andrey Shinkevich 
Message-id: 1559154027-282547-1-git-send-email-andrey.shinkev...@virtuozzo.com
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 hw/block/fdc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a64378f84b..77af9979de 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2648,6 +2648,7 @@ static void fdctrl_realize_common(DeviceState *dev, 
FDCtrl *fdctrl,
 
 FLOPPY_DPRINTF("init controller\n");
 fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
+memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
 fdctrl->fifo_size = 512;
 fdctrl->result_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
  fdctrl_result_timer, fdctrl);
-- 
2.21.0

[Qemu-block] [PULL 18/20] blkdebug: Inject errors on .bdrv_co_block_status()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190507203508.18026-6-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 5 -
 block/blkdebug.c | 8 
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 60f903afa3..61124431d8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3281,10 +3281,13 @@
 #
 # @flush: .bdrv_co_flush_to_disk()
 #
+# @block-status: .bdrv_co_block_status()
+#
 # Since: 4.1
 ##
 { 'enum': 'BlkdebugIOType', 'prefix': 'BLKDEBUG_IO_TYPE',
-  'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush' ] }
+  'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush',
+'block-status' ] }
 
 ##
 # @BlkdebugInjectErrorOptions:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 1663ed25af..5ae96c52b0 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -670,7 +670,15 @@ static int coroutine_fn 
blkdebug_co_block_status(BlockDriverState *bs,
  int64_t *map,
  BlockDriverState **file)
 {
+int err;
+
 assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));
+
+err = rule_check(bs, offset, bytes, BLKDEBUG_IO_TYPE_BLOCK_STATUS);
+if (err) {
+return err;
+}
+
 return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes,
   pnum, map, file);
 }
-- 
2.21.0

[Qemu-block] [PULL 20/20] iotests: Test qemu-img convert -C --salvage

[Max Reitz]

We do not support this combination (yet), so this should yield an error
message.

Signed-off-by: Max Reitz 
Tested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190507203508.18026-8-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/082 | 1 +
 tests/qemu-iotests/082.out | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index bbbdf555dc..3286c2c6db 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -162,6 +162,7 @@ echo === convert: -C and other options ===
 run_qemu_img convert -C -S 4k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
 run_qemu_img convert -C -S 8k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
 run_qemu_img convert -C -c -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+run_qemu_img convert -C --salvage -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
 
 echo
 echo === amend: Options specified more than once ===
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 7e25706813..58de358b38 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -567,6 +567,9 @@ qemu-img: Cannot enable copy offloading when -S is used
 Testing: convert -C -c -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
 qemu-img: Cannot enable copy offloading when -c is used
 
+Testing: convert -C --salvage -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot use copy offloading in salvaging mode
+
 === amend: Options specified more than once ===
 
 Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
-- 
2.21.0

Re: [Qemu-block] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Reopening a node's backing child needs a bit of special handling because
> the "backing" child has different defaults than all other children
> (among other things).  Adding filter support here is a bit more
> difficult than just using the child access functions.  In fact, we often
> have to directly use bs->backing because these functions are about the
> "backing" child (which may or may not be the COW backing file).
> 
> Signed-off-by: Max Reitz 
> ---
>   block.c | 36 +---
>   1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 505b3e9a01..db2759c10d 100644
> --- a/block.c
> +++ b/block.c
> @@ -3542,17 +3542,39 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
> *reopen_state,
>   }
>   }
>   
> +/*
> + * Ensure that @bs can really handle backing files, because we are
> + * about to give it one (or swap the existing one)
> + */
> +if (bs->drv->is_filter) {
> +/* Filters always have a file or a backing child */
> +if (!bs->backing) {
> +error_setg(errp, "'%s' is a %s filter node that does not support 
> a "
> +   "backing child", bs->node_name, bs->drv->format_name);
> +return -EINVAL;
> +}
> +} else if (!bs->drv->supports_backing) {
> +error_setg(errp, "Driver '%s' of node '%s' does not support backing "
> +   "files", bs->drv->format_name, bs->node_name);
> +return -EINVAL;
> +}

hmm, shouldn't we have these checks for overlay_bs?

> +
>   /*
>* Find the "actual" backing file by skipping all links that point
>* to an implicit node, if any (e.g. a commit filter node).
> + * We cannot use any of the bdrv_skip_*() functions here because
> + * those return the first explicit node, while we are looking for
> + * its overlay here.
>*/
>   overlay_bs = bs;
> -while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
> -overlay_bs = backing_bs(overlay_bs);
> +while (bdrv_filtered_bs(overlay_bs) &&
> +   bdrv_filtered_bs(overlay_bs)->implicit)
> +{
> +overlay_bs = bdrv_filtered_bs(overlay_bs);
>   }

here, overlay_bs may be some filter with file child ..

>   
>   /* If we want to replace the backing file we need some extra checks */
> -if (new_backing_bs != backing_bs(overlay_bs)) {
> +if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) {
>   /* Check for implicit nodes between bs and its backing file */
>   if (bs != overlay_bs) {
>   error_setg(errp, "Cannot change backing link if '%s' has "
> @@ -3560,8 +3582,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
> *reopen_state,
>   return -EPERM;
>   }
>   /* Check if the backing link that we want to replace is frozen */
> -if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
> - errp)) {
> +if (bdrv_is_backing_chain_frozen(overlay_bs,
> + child_bs(overlay_bs->backing), 
> errp)) {

.. and here we are doing wrong thing, as it don't have backing child

Aha, you use the fact that we now don't have implicit filters with file child. 
Then, should
we add an assertion for this?

>   return -EPERM;
>   }
>   reopen_state->replace_backing_bs = true;
> @@ -3712,7 +3734,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>* its metadata. Otherwise the 'backing' option can be omitted.
>*/
>   if (drv->supports_backing && reopen_state->backing_missing &&
> -(backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) 
> {
> +(reopen_state->bs->backing || reopen_state->bs->backing_file[0])) {
>   error_setg(errp, "backing is missing for '%s'",
>  reopen_state->bs->node_name);
>   ret = -EINVAL;
> @@ -3857,7 +3879,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>* from bdrv_set_backing_hd()) has the new values.
>*/
>   if (reopen_state->replace_backing_bs) {
> -BlockDriverState *old_backing_bs = backing_bs(bs);
> +BlockDriverState *old_backing_bs = child_bs(bs->backing);
>   assert(!old_backing_bs || !old_backing_bs->implicit);
>   /* Abort the permission update on the backing bs we're detaching */
>   if (old_backing_bs) {
> 


-- 
Best regards,
Vladimir

[Qemu-block] [PULL 14/20] qemu-img: Move quiet into ImgConvertState

[Max Reitz]

Move img_convert()'s quiet flag into the ImgConvertState so it is
accessible by nested functions.  -q dictates that it suppresses anything
but errors, so if those functions want to emit warnings, they need to
query this flag first.  (There currently are no such warnings, but there
will be as of the next patch.)

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190507203508.18026-2-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 qemu-img.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index da14aea46a..e15e617256 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1581,6 +1581,7 @@ typedef struct ImgConvertState {
 int64_t target_backing_sectors; /* negative if unknown */
 bool wr_in_order;
 bool copy_range;
+bool quiet;
 int min_sparse;
 int alignment;
 size_t cluster_sectors;
@@ -2012,7 +2013,7 @@ static int img_convert(int argc, char **argv)
 QDict *open_opts = NULL;
 char *options = NULL;
 Error *local_err = NULL;
-bool writethrough, src_writethrough, quiet = false, image_opts = false,
+bool writethrough, src_writethrough, image_opts = false,
  skip_create = false, progress = false, tgt_image_opts = false;
 int64_t ret = -EINVAL;
 bool force_share = false;
@@ -2120,7 +2121,7 @@ static int img_convert(int argc, char **argv)
 src_cache = optarg;
 break;
 case 'q':
-quiet = true;
+s.quiet = true;
 break;
 case 'n':
 skip_create = true;
@@ -2209,7 +2210,7 @@ static int img_convert(int argc, char **argv)
 }
 
 /* Initialize before goto out */
-if (quiet) {
+if (s.quiet) {
 progress = false;
 }
 qemu_progress_init(progress, 1.0);
@@ -2220,7 +2221,7 @@ static int img_convert(int argc, char **argv)
 
 for (bs_i = 0; bs_i < s.src_num; bs_i++) {
 s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
-   fmt, src_flags, src_writethrough, quiet,
+   fmt, src_flags, src_writethrough, s.quiet,
force_share);
 if (!s.src[bs_i]) {
 ret = -1;
@@ -2383,7 +2384,7 @@ static int img_convert(int argc, char **argv)
 
 if (skip_create) {
 s.target = img_open(tgt_image_opts, out_filename, out_fmt,
-flags, writethrough, quiet, false);
+flags, writethrough, s.quiet, false);
 } else {
 /* TODO ultimately we should allow --target-image-opts
  * to be used even when -n is not given.
@@ -2391,7 +2392,7 @@ static int img_convert(int argc, char **argv)
  * to allow filenames in option syntax
  */
 s.target = img_open_file(out_filename, open_opts, out_fmt,
- flags, writethrough, quiet, false);
+ flags, writethrough, s.quiet, false);
 open_opts = NULL; /* blk_new_open will have freed it */
 }
 if (!s.target) {
-- 
2.21.0

[Qemu-block] [PULL 11/20] qemu-img: Fix options leakage in img_rebase()

[Max Reitz]

img_rebase() can leak a QDict in two occasions.  Fix it.

Coverity: CID 1401416
Fixes: d16699b64671466b42079c45b89127aeea1ca565
Fixes: 330c72957196e0ae382abcaa97ebf4eb9bc8574f
Signed-off-by: Max Reitz 
Message-id: 20190528195338.12376-1-mre...@redhat.com
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 qemu-img.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index fd62e3ad5d..da14aea46a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3350,6 +3350,7 @@ static int img_rebase(int argc, char **argv)
  out_baseimg,
  _err);
 if (local_err) {
+qobject_unref(options);
 error_reportf_err(local_err,
   "Could not resolve backing filename: ");
 ret = -1;
@@ -3362,7 +3363,9 @@ static int img_rebase(int argc, char **argv)
  */
 prefix_chain_bs = bdrv_find_backing_image(bs, out_real_path);
 if (prefix_chain_bs) {
+qobject_unref(options);
 g_free(out_real_path);
+
 blk_new_backing = blk_new(qemu_get_aio_context(),
   BLK_PERM_CONSISTENT_READ,
   BLK_PERM_ALL);
-- 
2.21.0

[Qemu-block] [PULL 04/20] iotests.py: rewrite run_job to be pickier

[Max Reitz]

From: John Snow 

Don't pull events out of the queue that don't belong to us;
be choosier so that we can use this method to drive jobs that
were launched by transactions that may have more jobs.

Signed-off-by: John Snow 
Message-id: 20190523170643.20794-5-js...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 48 +--
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6a3703e6ee..3ecef5bc90 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -543,27 +543,37 @@ class VM(qtest.QEMUQtestMachine):
 # Returns None on success, and an error string on failure
 def run_job(self, job, auto_finalize=True, auto_dismiss=False,
 pre_finalize=None, wait=60.0):
+match_device = {'data': {'device': job}}
+match_id = {'data': {'id': job}}
+events = [
+('BLOCK_JOB_COMPLETED', match_device),
+('BLOCK_JOB_CANCELLED', match_device),
+('BLOCK_JOB_ERROR', match_device),
+('BLOCK_JOB_READY', match_device),
+('BLOCK_JOB_PENDING', match_id),
+('JOB_STATUS_CHANGE', match_id)
+]
 error = None
 while True:
-for ev in self.get_qmp_events_filtered(wait=wait):
-if ev['event'] == 'JOB_STATUS_CHANGE':
-status = ev['data']['status']
-if status == 'aborting':
-result = self.qmp('query-jobs')
-for j in result['return']:
-if j['id'] == job:
-error = j['error']
-log('Job failed: %s' % (j['error']))
-elif status == 'pending' and not auto_finalize:
-if pre_finalize:
-pre_finalize()
-self.qmp_log('job-finalize', id=job)
-elif status == 'concluded' and not auto_dismiss:
-self.qmp_log('job-dismiss', id=job)
-elif status == 'null':
-return error
-else:
-log(ev)
+ev = filter_qmp_event(self.events_wait(events))
+if ev['event'] != 'JOB_STATUS_CHANGE':
+log(ev)
+continue
+status = ev['data']['status']
+if status == 'aborting':
+result = self.qmp('query-jobs')
+for j in result['return']:
+if j['id'] == job:
+error = j['error']
+log('Job failed: %s' % (j['error']))
+elif status == 'pending' and not auto_finalize:
+if pre_finalize:
+pre_finalize()
+self.qmp_log('job-finalize', id=job)
+elif status == 'concluded' and not auto_dismiss:
+self.qmp_log('job-dismiss', id=job)
+elif status == 'null':
+return error
 
 def node_info(self, node_name):
 nodes = self.qmp('query-named-block-nodes')
-- 
2.21.0

[Qemu-block] [PULL 01/20] blockdev-backup: don't check aio_context too early

[Max Reitz]

From: John Snow 

in blockdev_backup_prepare, we check to make sure that the target is
associated with a compatible aio context. However, do_blockdev_backup is
called later and has some logic to move the target to a compatible
aio_context. The transaction version will fail certain commands
needlessly early as a result.

Allow blockdev_backup_prepare to simply call do_blockdev_backup, which
will ultimately decide if the contexts are compatible or not.

Note: the transaction version has always disallowed this operation since
its initial commit bd8baecd (2014), whereas the version of
qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to
enforce the aio_context switch instead. It's not clear, and I can't see
from the mailing list archives at the time, why the two functions take a
different approach. It wasn't until later in efd7556708b (2016) that the
standalone version tried to determine if it could set the context or
not.

Reported-by: aihua liang 
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498
Signed-off-by: John Snow 
Message-id: 20190523170643.20794-2-js...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 blockdev.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3f44b891eb..fdafa173cc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1876,10 +1876,6 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)
 }
 
 aio_context = bdrv_get_aio_context(bs);
-if (aio_context != bdrv_get_aio_context(target)) {
-error_setg(errp, "Backup between two IO threads is not implemented");
-return;
-}
 aio_context_acquire(aio_context);
 state->bs = bs;
 
-- 
2.21.0

[Qemu-block] [PULL 12/20] qapi/block-core: Overlays are not snapshots

[Max Reitz]

A snapshot is something that reflects the state of something at a
certain point in time.  It does not change.

The file our snapshot commands create (or the node they install) is not
a snapshot, as it does change over time.  It is an overlay.  We cannot
do anything about the parameter names, but we can at least adjust the
descriptions to reflect that fact.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Message-id: 20190603202236.1342-2-mre...@redhat.com
Reviewed-by: John Snow 
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index fcd054fcb1..c0ff3a83ef 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1279,17 +1279,17 @@
 #
 # Either @device or @node-name must be set but not both.
 #
-# @device: the name of the device to generate the snapshot from.
+# @device: the name of the device to take a snapshot of.
 #
 # @node-name: graph node name to generate the snapshot from (Since 2.0)
 #
-# @snapshot-file: the target of the new image. If the file exists, or
-# if it is a device, the snapshot will be created in the existing
-# file/device. Otherwise, a new file will be created.
+# @snapshot-file: the target of the new overlay image. If the file
+# exists, or if it is a device, the overlay will be created in the
+# existing file/device. Otherwise, a new file will be created.
 #
 # @snapshot-node-name: the graph node name of the new image (Since 2.0)
 #
-# @format: the format of the snapshot image, default is 'qcow2'.
+# @format: the format of the overlay image, default is 'qcow2'.
 #
 # @mode: whether and how QEMU should create a new image, default is
 #'absolute-paths'.
@@ -1302,10 +1302,10 @@
 ##
 # @BlockdevSnapshot:
 #
-# @node: device or node name that will have a snapshot created.
+# @node: device or node name that will have a snapshot taken.
 #
 # @overlay: reference to the existing block device that will become
-#   the overlay of @node, as part of creating the snapshot.
+#   the overlay of @node, as part of taking the snapshot.
 #   It must not have a current backing file (this can be
 #   achieved by passing "backing": null to blockdev-add).
 #
@@ -1443,7 +1443,7 @@
 ##
 # @blockdev-snapshot-sync:
 #
-# Generates a synchronous snapshot of a block device.
+# Takes a synchronous snapshot of a block device.
 #
 # For the arguments, see the documentation of BlockdevSnapshotSync.
 #
@@ -1469,9 +1469,9 @@
 ##
 # @blockdev-snapshot:
 #
-# Generates a snapshot of a block device.
+# Takes a snapshot of a block device.
 #
-# Create a snapshot, by installing 'node' as the backing image of
+# Take a snapshot, by installing 'node' as the backing image of
 # 'overlay'. Additionally, if 'node' is associated with a block
 # device, the block device changes to using 'overlay' as its new active
 # image.
-- 
2.21.0

[Qemu-block] [PULL 05/20] iotests: add iotest 256 for testing blockdev-backup across iothread contexts

[Max Reitz]

From: John Snow 

Signed-off-by: John Snow 
Message-id: 20190523170643.20794-6-js...@redhat.com
Reviewed-by: Max Reitz 
[mreitz: Moved from 250 to 256]
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/256 | 122 +
 tests/qemu-iotests/256.out | 119 
 tests/qemu-iotests/group   |   1 +
 3 files changed, 242 insertions(+)
 create mode 100755 tests/qemu-iotests/256
 create mode 100644 tests/qemu-iotests/256.out

diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
new file mode 100755
index 00..c594a43205
--- /dev/null
+++ b/tests/qemu-iotests/256
@@ -0,0 +1,122 @@
+#!/usr/bin/env python
+#
+# Test incremental/backup across iothread contexts
+#
+# Copyright (c) 2019 John Snow for 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 .
+#
+# owner=js...@redhat.com
+
+import os
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+size = 64 * 1024 * 1024
+
+with iotests.FilePath('img0') as img0_path, \
+ iotests.FilePath('img1') as img1_path, \
+ iotests.FilePath('img0-full') as img0_full_path, \
+ iotests.FilePath('img1-full') as img1_full_path, \
+ iotests.FilePath('img0-incr') as img0_incr_path, \
+ iotests.FilePath('img1-incr') as img1_incr_path, \
+ iotests.VM() as vm:
+
+def create_target(filepath, name, size):
+basename = os.path.basename(filepath)
+nodename = "file_{}".format(basename)
+log(vm.command('blockdev-create', job_id='job1',
+   options={
+   'driver': 'file',
+   'filename': filepath,
+   'size': 0,
+   }))
+vm.run_job('job1')
+log(vm.command('blockdev-add', driver='file',
+   node_name=nodename, filename=filepath))
+log(vm.command('blockdev-create', job_id='job2',
+   options={
+   'driver': iotests.imgfmt,
+   'file': nodename,
+   'size': size,
+   }))
+vm.run_job('job2')
+log(vm.command('blockdev-add', driver=iotests.imgfmt,
+   node_name=name,
+   file=nodename))
+
+log('--- Preparing images & VM ---\n')
+vm.add_object('iothread,id=iothread0')
+vm.add_object('iothread,id=iothread1')
+vm.add_device('virtio-scsi-pci,id=scsi0,iothread=iothread0')
+vm.add_device('virtio-scsi-pci,id=scsi1,iothread=iothread1')
+iotests.qemu_img_create('-f', iotests.imgfmt, img0_path, str(size))
+iotests.qemu_img_create('-f', iotests.imgfmt, img1_path, str(size))
+vm.add_drive(img0_path, interface='none')
+vm.add_device('scsi-hd,id=device0,drive=drive0,bus=scsi0.0')
+vm.add_drive(img1_path, interface='none')
+vm.add_device('scsi-hd,id=device1,drive=drive1,bus=scsi1.0')
+
+log('--- Starting VM ---\n')
+vm.launch()
+
+log('--- Create Targets & Full Backups ---\n')
+create_target(img0_full_path, 'img0-full', size)
+create_target(img1_full_path, 'img1-full', size)
+ret = vm.qmp_log('transaction', indent=2, actions=[
+{ 'type': 'block-dirty-bitmap-add',
+  'data': { 'node': 'drive0', 'name': 'bitmap0' }},
+{ 'type': 'block-dirty-bitmap-add',
+  'data': { 'node': 'drive1', 'name': 'bitmap1' }},
+{ 'type': 'blockdev-backup',
+  'data': { 'device': 'drive0',
+'target': 'img0-full',
+'sync': 'full',
+'job-id': 'j0' }},
+{ 'type': 'blockdev-backup',
+  'data': { 'device': 'drive1',
+'target': 'img1-full',
+'sync': 'full',
+'job-id': 'j1' }}
+])
+if "error" in ret:
+raise Exception(ret['error']['desc'])
+vm.run_job('j0', auto_dismiss=True)
+vm.run_job('j1', auto_dismiss=True)
+
+log('\n--- Create Targets & Incremental Backups ---\n')
+create_target(img0_incr_path, 'img0-incr', size)
+create_target(img1_incr_path, 'img1-incr', size)
+ret = vm.qmp_log('transaction', indent=2, actions=[
+{ 'type': 'blockdev-backup',
+  'data': { 'device': 'drive0',
+'target': 'img0-incr',
+'sync': 

[Qemu-block] [PULL 08/20] iotests: Fix intermittent failure in 219

[Max Reitz]

In 219, we wait for the job to make progress before we emit its status.
This makes the output reliable.  We do not wait for any more progress if
the job's current-progress already matches its total-progress.

Unfortunately, there is a bug: Right after the job has been started,
it's possible that total-progress is still 0.  In that case, we may skip
the first progress-making step and keep ending up 64 kB short.

To fix that bug, we can simply wait for total-progress to reach 4 MB
(the image size) after starting the job.

Reported-by: Karen Mezick 
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1686651
Signed-off-by: Max Reitz 
Message-id: 20190516161114.27596-1-mre...@redhat.com
Reviewed-by: John Snow 
[mreitz: Adjusted commit message as per John's proposal]
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/219 | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index c03bbdb294..e0c51662c0 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -23,6 +23,8 @@ import iotests
 
 iotests.verify_image_format(supported_fmts=['qcow2'])
 
+img_size = 4 * 1024 * 1024
+
 def pause_wait(vm, job_id):
 with iotests.Timeout(3, "Timeout waiting for job to pause"):
 while True:
@@ -62,6 +64,8 @@ def test_pause_resume(vm):
 iotests.log(vm.qmp('query-jobs'))
 
 def test_job_lifecycle(vm, job, job_args, has_ready=False):
+global img_size
+
 iotests.log('')
 iotests.log('')
 iotests.log('Starting block job: %s (auto-finalize: %s; auto-dismiss: %s)' 
%
@@ -84,6 +88,10 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False):
 iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
 iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
 
+# Wait for total-progress to stabilize
+while vm.qmp('query-jobs')['return'][0]['total-progress'] < img_size:
+pass
+
 # RUNNING state:
 # pause/resume should work, complete/finalize/dismiss should error out
 iotests.log('')
@@ -173,9 +181,8 @@ with iotests.FilePath('disk.img') as disk_path, \
  iotests.FilePath('copy.img') as copy_path, \
  iotests.VM() as vm:
 
-img_size = '4M'
-iotests.qemu_img_create('-f', iotests.imgfmt, disk_path, img_size)
-iotests.qemu_io('-c', 'write 0 %s' % (img_size),
+iotests.qemu_img_create('-f', iotests.imgfmt, disk_path, str(img_size))
+iotests.qemu_io('-c', 'write 0 %i' % (img_size),
 '-f', iotests.imgfmt, disk_path)
 
 iotests.log('Launching VM...')
-- 
2.21.0

[Qemu-block] [PULL 03/20] QEMUMachine: add events_wait method

[Max Reitz]

From: John Snow 

Instead of event_wait which looks for a single event, add an events_wait
which can look for any number of events simultaneously. However, it
will still only return one at a time, whichever happens first.

Signed-off-by: John Snow 
Message-id: 20190523170643.20794-4-js...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 python/qemu/__init__.py | 69 +
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 81d9657ec0..98ed8a2e28 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -402,42 +402,71 @@ class QEMUMachine(object):
 self._qmp.clear_events()
 return events
 
-def event_wait(self, name, timeout=60.0, match=None):
+@staticmethod
+def event_match(event, match=None):
 """
-Wait for specified timeout on named event in QMP; optionally filter
-results by match.
+Check if an event matches optional match criteria.
 
-The 'match' is checked to be a recursive subset of the 'event'; skips
-branch processing on match's value None
-   {"foo": {"bar": 1}} matches {"foo": None}
-   {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
+The match criteria takes the form of a matching subdict. The event is
+checked to be a superset of the subdict, recursively, with matching
+values whenever those values are not None.
+
+Examples, with the subdict queries on the left:
+ - None matches any object.
+ - {"foo": None} matches {"foo": {"bar": 1}}
+ - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
+ - {"foo": {"baz": 2}} matches {"foo": {"bar": 1, "baz": 2}}
 """
-def event_match(event, match=None):
-if match is None:
-return True
+if match is None:
+return True
 
-for key in match:
-if key in event:
-if isinstance(event[key], dict):
-if not event_match(event[key], match[key]):
-return False
-elif event[key] != match[key]:
+for key in match:
+if key in event:
+if isinstance(event[key], dict):
+if not QEMUMachine.event_match(event[key], match[key]):
 return False
-else:
+elif event[key] != match[key]:
 return False
+else:
+return False
+return True
 
-return True
+def event_wait(self, name, timeout=60.0, match=None):
+"""
+event_wait waits for and returns a named event from QMP with a timeout.
+
+name: The event to wait for.
+timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+match: Optional match criteria. See event_match for details.
+"""
+return self.events_wait([(name, match)], timeout)
+
+def events_wait(self, events, timeout=60.0):
+"""
+events_wait waits for and returns a named event from QMP with a 
timeout.
+
+events: a sequence of (name, match_criteria) tuples.
+The match criteria are optional and may be None.
+See event_match for details.
+timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+"""
+def _match(event):
+for name, match in events:
+if (event['event'] == name and
+self.event_match(event, match)):
+return True
+return False
 
 # Search cached events
 for event in self._events:
-if (event['event'] == name) and event_match(event, match):
+if _match(event):
 self._events.remove(event)
 return event
 
 # Poll for new events
 while True:
 event = self._qmp.pull_event(wait=timeout)
-if (event['event'] == name) and event_match(event, match):
+if _match(event):
 return event
 self._events.append(event)
 
-- 
2.21.0

[Qemu-block] [PULL 02/20] iotests.py: do not use infinite waits

[Max Reitz]

From: John Snow 

Cap waits to 60 seconds so that iotests can fail gracefully if something
goes wrong.

Signed-off-by: John Snow 
Message-id: 20190523170643.20794-3-js...@redhat.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f11482f3dc..6a3703e6ee 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -524,7 +524,7 @@ class VM(qtest.QEMUQtestMachine):
 output_list += [key + '=' + obj[key]]
 return ','.join(output_list)
 
-def get_qmp_events_filtered(self, wait=True):
+def get_qmp_events_filtered(self, wait=60.0):
 result = []
 for ev in self.get_qmp_events(wait=wait):
 result.append(filter_qmp_event(ev))
@@ -542,10 +542,10 @@ class VM(qtest.QEMUQtestMachine):
 
 # Returns None on success, and an error string on failure
 def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-pre_finalize=None):
+pre_finalize=None, wait=60.0):
 error = None
 while True:
-for ev in self.get_qmp_events_filtered(wait=True):
+for ev in self.get_qmp_events_filtered(wait=wait):
 if ev['event'] == 'JOB_STATUS_CHANGE':
 status = ev['data']['status']
 if status == 'aborting':
@@ -650,7 +650,7 @@ class QMPTestCase(unittest.TestCase):
 
self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
  self.vm.flatten_qmp_object(reference))
 
-def cancel_and_wait(self, drive='drive0', force=False, resume=False):
+def cancel_and_wait(self, drive='drive0', force=False, resume=False, 
wait=60.0):
 '''Cancel a block job and wait for it to finish, returning the event'''
 result = self.vm.qmp('block-job-cancel', device=drive, force=force)
 self.assert_qmp(result, 'return', {})
@@ -661,7 +661,7 @@ class QMPTestCase(unittest.TestCase):
 cancelled = False
 result = None
 while not cancelled:
-for event in self.vm.get_qmp_events(wait=True):
+for event in self.vm.get_qmp_events(wait=wait):
 if event['event'] == 'BLOCK_JOB_COMPLETED' or \
event['event'] == 'BLOCK_JOB_CANCELLED':
 self.assert_qmp(event, 'data/device', drive)
@@ -674,10 +674,10 @@ class QMPTestCase(unittest.TestCase):
 self.assert_no_active_block_jobs()
 return result
 
-def wait_until_completed(self, drive='drive0', check_offset=True):
+def wait_until_completed(self, drive='drive0', check_offset=True, 
wait=60.0):
 '''Wait for a block job to finish, returning the event'''
 while True:
-for event in self.vm.get_qmp_events(wait=True):
+for event in self.vm.get_qmp_events(wait=wait):
 if event['event'] == 'BLOCK_JOB_COMPLETED':
 self.assert_qmp(event, 'data/device', drive)
 self.assert_qmp_absent(event, 'data/error')
-- 
2.21.0

[Qemu-block] [PULL 06/20] event_match: always match on None value

[Max Reitz]

From: John Snow 

Before, event_match didn't always recurse if the event value was not a
dictionary, and would instead check for equality immediately.

By delaying equality checking to post-recursion, we can allow leaf
values like "5" to match "None" and take advantage of the generic
None-returns-True clause.

This makes the matching a little more obviously consistent at the
expense of being able to check for explicit None values, which is
probably not that important given what this function is used for.

Signed-off-by: John Snow 
Message-id: 20190528183857.26167-1-js...@redhat.com
Signed-off-by: Max Reitz 
---
 python/qemu/__init__.py | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 98ed8a2e28..dbaf8a5311 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -409,27 +409,31 @@ class QEMUMachine(object):
 
 The match criteria takes the form of a matching subdict. The event is
 checked to be a superset of the subdict, recursively, with matching
-values whenever those values are not None.
+values whenever the subdict values are not None.
+
+This has a limitation that you cannot explicitly check for None values.
 
 Examples, with the subdict queries on the left:
  - None matches any object.
  - {"foo": None} matches {"foo": {"bar": 1}}
- - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
- - {"foo": {"baz": 2}} matches {"foo": {"bar": 1, "baz": 2}}
+ - {"foo": None} matches {"foo": 5}
+ - {"foo": {"abc": None}} does not match {"foo": {"bar": 1}}
+ - {"foo": {"rab": 2}} matches {"foo": {"bar": 1, "rab": 2}}
 """
 if match is None:
 return True
 
-for key in match:
-if key in event:
-if isinstance(event[key], dict):
+try:
+for key in match:
+if key in event:
 if not QEMUMachine.event_match(event[key], match[key]):
 return False
-elif event[key] != match[key]:
+else:
 return False
-else:
-return False
-return True
+return True
+except TypeError:
+# either match or event wasn't iterable (not a dict)
+return match == event
 
 def event_wait(self, name, timeout=60.0, match=None):
 """
-- 
2.21.0

[Qemu-block] [PULL 00/20] Block patches

[Max Reitz]

The following changes since commit 5ec2eca83dc478ddf24077e02a8b34dd26cd3ff9:

  Merge remote-tracking branch 'remotes/awilliam/tags/vfio-updates-20190613.0' 
into staging (2019-06-14 09:33:55 +0100)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2019-06-14

for you to fetch changes up to 21c1ce592a144188dfe59b9e156a97da412a59a2:

  iotests: Test qemu-img convert -C --salvage (2019-06-14 15:09:42 +0200)


Block patches:
- Allow blockdev-backup from nodes that are not in qemu's main AIO
  context to newly added nodes
- Add salvaging mode to qemu-img convert
- Minor fixes to tests, documentation, and for less Valgrind annoyance


Andrey Shinkevich (1):
  hw/block/fdc: floppy command FIFO memory initialization

John Snow (6):
  blockdev-backup: don't check aio_context too early
  iotests.py: do not use infinite waits
  QEMUMachine: add events_wait method
  iotests.py: rewrite run_job to be pickier
  iotests: add iotest 256 for testing blockdev-backup across iothread
contexts
  event_match: always match on None value

Max Reitz (12):
  iotests: Filter 175's allocation information
  iotests: Fix intermittent failure in 219
  qemu-img: Fix options leakage in img_rebase()
  qapi/block-core: Overlays are not snapshots
  blockdev: Overlays are not snapshots
  qemu-img: Move quiet into ImgConvertState
  qemu-img: Add salvaging mode to convert
  blkdebug: Add @iotype error option
  blkdebug: Add "none" event
  blkdebug: Inject errors on .bdrv_co_block_status()
  iotests: Test qemu-img convert --salvage
  iotests: Test qemu-img convert -C --salvage

Vladimir Sementsov-Ogievskiy (1):
  iotests: restrict 254 to support only qcow2

 qapi/block-core.json  |  53 ---
 block/blkdebug.c  |  60 ++--
 blockdev.c|  14 +--
 hw/block/fdc.c|   1 +
 qemu-img.c| 106 +++--
 python/qemu/__init__.py   |  67 ++
 qemu-img-cmds.hx  |   4 +-
 qemu-img.texi |   4 +
 tests/qemu-iotests/082|   1 +
 tests/qemu-iotests/082.out|   3 +
 tests/qemu-iotests/085.out|  10 +-
 tests/qemu-iotests/175|  26 +-
 tests/qemu-iotests/175.out|   8 +-
 tests/qemu-iotests/219|  13 ++-
 tests/qemu-iotests/251| 170 ++
 tests/qemu-iotests/251.out|  43 +
 tests/qemu-iotests/254|   2 +
 tests/qemu-iotests/256| 122 
 tests/qemu-iotests/256.out| 119 
 tests/qemu-iotests/group  |   2 +
 tests/qemu-iotests/iotests.py |  60 +++-
 21 files changed, 772 insertions(+), 116 deletions(-)
 create mode 100755 tests/qemu-iotests/251
 create mode 100644 tests/qemu-iotests/251.out
 create mode 100755 tests/qemu-iotests/256
 create mode 100644 tests/qemu-iotests/256.out

-- 
2.21.0

Re: [Qemu-block] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Use child access functions when iterating through backing chains so
> filters do not break the chain.
> 
> Signed-off-by: Max Reitz 
> ---
>   block.c | 40 
>   1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 11f37983d9..505b3e9a01 100644
> --- a/block.c
> +++ b/block.c
> @@ -4261,7 +4261,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>   }
>   
>   /*
> - * Finds the image layer in the chain that has 'bs' as its backing file.
> + * Finds the image layer in the chain that has 'bs' (or a filter on
> + * top of it) as its backing file.
>*
>* active is the current topmost image.
>*
> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>   BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>   BlockDriverState *bs)
>   {
> -while (active && bs != backing_bs(active)) {
> -active = backing_bs(active);
> +bs = bdrv_skip_rw_filters(bs);
> +active = bdrv_skip_rw_filters(active);
> +
> +while (active) {
> +BlockDriverState *next = bdrv_backing_chain_next(active);
> +if (bs == next) {
> +return active;
> +}
> +active = next;
>   }
>   
> -return active;
> +return NULL;
>   }

Semantics changed for this function.
It is used in two places
1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we will 
never have
filter node as a bottom of some valid chain

2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't 
understand,
why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs overlay 
is out of the job,
what is this check for?


>   
>   /* Given a BDS, searches for the base layer. */
> @@ -4421,9 +4429,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
> BlockDriverState *base,
>* other intermediate nodes have been dropped.
>* If 'top' is an implicit node (e.g. "commit_top") we should skip
>* it because no one inherits from it. We use explicit_top for that. */
> -while (explicit_top && explicit_top->implicit) {
> -explicit_top = backing_bs(explicit_top);
> -}
> +explicit_top = bdrv_skip_implicit_filters(explicit_top);
>   update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top);
>   
>   /* success - we can delete the intermediate states, and link top->base 
> */
> @@ -4902,7 +4908,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
>   bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
>   {
>   while (top && top != base) {
> -top = backing_bs(top);
> +top = bdrv_filtered_bs(top);
>   }
>   
>   return top != NULL;
> @@ -5141,7 +5147,17 @@ BlockDriverState 
> *bdrv_find_backing_image(BlockDriverState *bs,
>   
>   is_protocol = path_has_protocol(backing_file);
>   
> -for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) {
> +/*
> + * Being largely a legacy function, skip any filters here
> + * (because filters do not have normal filenames, so they cannot
> + * match anyway; and allowing json:{} filenames is a bit out of
> + * scope).
> + */
> +for (curr_bs = bdrv_skip_rw_filters(bs);
> + bdrv_filtered_cow_child(curr_bs) != NULL;
> + curr_bs = bdrv_backing_chain_next(curr_bs))
> +{
> +BlockDriverState *bs_below = bdrv_backing_chain_next(curr_bs);
>   
>   /* If either of the filename paths is actually a protocol, then
>* compare unmodified paths; otherwise make paths relative */
> @@ -5149,7 +5165,7 @@ BlockDriverState 
> *bdrv_find_backing_image(BlockDriverState *bs,
>   char *backing_file_full_ret;
>   
>   if (strcmp(backing_file, curr_bs->backing_file) == 0) {

hmm, interesting, what bs->backing_file now means? It's strange enough to store 
such field on
bds, when we have backing link anyway..

> -retval = curr_bs->backing->bs;
> +retval = bs_below;
>   break;
>   }
>   /* Also check against the full backing filename for the image */
> @@ -5159,7 +5175,7 @@ BlockDriverState 
> *bdrv_find_backing_image(BlockDriverState *bs,
>   bool equal = strcmp(backing_file, backing_file_full_ret) == 
> 0;
>   g_free(backing_file_full_ret);
>   if (equal) {
> -retval = curr_bs->backing->bs;
> +retval = bs_below;
>   break;
>   }
>   }
> @@ -5185,7 +5201,7 @@ BlockDriverState 
> *bdrv_find_backing_image(BlockDriverState *bs,
>   g_free(filename_tmp);
>   
>   if (strcmp(backing_file_full, filename_full) == 0) {
> -retval = curr_bs->backing->bs;
> +retval = bs_below;
> 

Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver

[Max Reitz]

On 14.06.19 11:04, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 18:57, Max Reitz wrote:
>> On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
>>> Backup-top filter does copy-before-write operation. It should be
>>> inserted above active disk and has a target node for CBW, like the
>>> following:
>>>
>>>  +---+
>>>  | Guest |
>>>  +---+
>>>  |r,w
>>>  v
>>>  ++  target   +---+
>>>  | backup_top |-->| target(qcow2) |
>>>  ++   CBW +---+
>>>  |
>>> backing |r,w
>>>  v
>>>  +-+
>>>  | Active disk |
>>>  +-+
>>>
>>> The driver will be used in backup instead of write-notifiers.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/backup-top.h  |  64 +
>>>   block/backup-top.c  | 322 
>>>   block/Makefile.objs |   2 +
>>>   3 files changed, 388 insertions(+)
>>>   create mode 100644 block/backup-top.h
>>>   create mode 100644 block/backup-top.c
>>>
>>> diff --git a/block/backup-top.h b/block/backup-top.h
>>> new file mode 100644
>>> index 00..788e18c358
>>> --- /dev/null
>>> +++ b/block/backup-top.h

[...]

>>> +/*
>>> + * bdrv_backup_top_append
>>> + *
>>> + * Append backup_top filter node above @source node. @target node will 
>>> receive
>>> + * the data backed up during CBE operations. New filter together with 
>>> @target
>>> + * node are attached to @source aio context.
>>> + *
>>> + * The resulting filter node is implicit.
>>
>> Why?  It’s just as easy for the caller to just make it implicit if it
>> should be.  (And usually the caller should decide that.)
> 
> Personally, I don't know what are the reasons for filters to bi implicit or 
> not,
> I just made it like other job-filters.. I can move making-implicit to the 
> caller
> or drop it at all (if it will work).

Nodes are implicit if they haven’t been added consciously by the user.
A node added by a block job can be non-implicit, too, as mirror shows;
If the user specifies the filter-node-name option, they will know about
the node, thus it is no longer implicit.

If the user doesn’t know about the node (they didn’t give the
filter-node-name option), the node is implicit.

[...]

>>> +static int coroutine_fn backup_top_co_flush(BlockDriverState *bs)
>>> +{
>>> +if (!bs->backing) {
>>> +return 0;
>>> +}
>>> +
>>> +return bdrv_co_flush(bs->backing->bs);
>>
>> Should we flush the target, too?
> 
> Hm, you've asked it already, on previous version :)

I wasn’t sure...

> Backup don't do it,
> so I just keep old behavior. And what is the reason to flush backup target
> on any guest flush?

Hm, well, what’s the reason not to do it?
Also, there are not only guest flushes.  bdrv_flush_all() exists, which
is called when the guest is stopped.  So who is going to flush the
target if not its parent?

[...]

>>> +
>>> +if (role == _file) {
>>> +/*
>>> + * Target child
>>> + *
>>> + * Share write to target (child_file), to not interfere
>>> + * with guest writes to its disk which may be in target backing 
>>> chain.
>>> + */
>>> +if (perm & BLK_PERM_WRITE) {
>>> +*nshared = *nshared | BLK_PERM_WRITE;
>>
>> Why not always share WRITE on the target?
> 
> Hmm, it's a bad thing to share writes on target, so I'm trying to reduce 
> number of
> cases when we have share it.

Is it?  First of all, this filter doesn’t care.  It doesn’t even read
from the target (related note: we probably don’t need CONSISTENT_READ on
the target).

Second, there is generally going to be a parent on backup-top that has
the WRITE permission taken.  So this does not really effectively reduce
that number of cases.

>>> +}
>>> +} else {
>>> +/* Source child */
>>> +if (perm & BLK_PERM_WRITE) {
>>
>> Or WRITE_UNCHANGED, no?
> 
> Why? We don't need doing CBW for unchanged write.

But we will do it still, won’t we?

(If an unchanging write comes in, this driver will handle it just like a
normal write, will it not?)

[...]

>>> +
>>> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>> + BlockDriverState *target,
>>> + HBitmap *copy_bitmap,
>>> + Error **errp)
>>> +{

[...]

>>> +}
>>
>> I guess it would be nice if it users could blockdev-add a backup-top
>> node to basically get a backup with sync=none.
>>
>> (The bdrv_open implementation should then create a new bitmap and
>> initialize it to be fully set.)
>>
>> But maybe it wouldn’t be so nice and I just have feverish dreams.
> 
> When series begun, I was trying to make exactly this - user-available filter,
> which may be used in separate, but you was against)

Past me is stupid.

> Maybe, not totally against, but I decided not to argue long, and 

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/14/19 2:29 PM, Stefan Weil wrote:
> On 14.06.19 12:13, Philippe Mathieu-Daudé wrote:
>> I agree with Kevin. The only user of the 'ssh' block driver that I am
>> aware of is the virt-v2v tool:
>>
>> http://libguestfs.org/virt-v2v.1.html#convert-from-esxi-hypervisor-over-ssh-to-local-libvirt
>>
>> Stefan, do you think someone would use it on a Windows host?
> 
> 
> I simply don't know. Typically people contact me if something does not
> work. Rarely they send an e-mail to say what they do and that everything
> is fine.
> 
> If QEMU for Windows builds without libssl, I'll build binaries without

"libssh" ;)

> it, add that information to the release notes and wait what happens.
> 
> So no objection from my side.

Glad to hear, thanks!

Pino: Can you improve the commit message to explain that QEMU only uses
libssh for the 'ssh block driver'? Maybe you (or Kevin/Max) can also add
a 1 line description of what is a 'block driver', so reviewer are not
worried that a feature might be removed.

Thanks!

Phil.

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Stefan Weil]

On 14.06.19 12:13, Philippe Mathieu-Daudé wrote:
> I agree with Kevin. The only user of the 'ssh' block driver that I am
> aware of is the virt-v2v tool:
> 
> http://libguestfs.org/virt-v2v.1.html#convert-from-esxi-hypervisor-over-ssh-to-local-libvirt
> 
> Stefan, do you think someone would use it on a Windows host?


I simply don't know. Typically people contact me if something does not
work. Rarely they send an e-mail to say what they do and that everything
is fine.

If QEMU for Windows builds without libssl, I'll build binaries without
it, add that information to the release notes and wait what happens.

So no objection from my side.

Cheers
Stefan

Re: [Qemu-block] [PATCH v5 13/42] block: Use CAFs in block status functions

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Use the child access functions in the block status inquiry functions as
> appropriate.
> 
> Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy 

-- 
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v3 15/15] vl: Deprecate -mon pretty=... for HMP monitors

[Markus Armbruster]

Kevin Wolf  writes:

> Am 14.06.2019 um 11:01 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > The -mon pretty=on|off switch of the -mon option applies only the QMP
>
> s/the QMP/to QMP/
>
> Can you fix up this one as well while you're at it?

Sure!

>> > monitors. It used to be silently ignored for HMP. Deprecate this
>> > combination so that we can make it an error in future versions.
>> >
>> > Signed-off-by: Kevin Wolf 

Re: [Qemu-block] [Qemu-devel] [PATCH] block/replication.c: Fix crash issue after failover

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190614092853.26551-1-chen.zh...@intel.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

PASS 32 test-opts-visitor /visitor/opts/range/beyond
PASS 33 test-opts-visitor /visitor/opts/dict/unvisited
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-coroutine -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="test-coroutine" 
==7787==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
==7787==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 
0x7ffef4e8; bottom 0x7fbf709f8000; size: 0x003f84488000 (272802283520)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
PASS 1 test-coroutine /basic/no-dangling-access
---
PASS 1 fdc-test /x86_64/fdc/cmos
PASS 2 fdc-test /x86_64/fdc/no_media_on_start
PASS 3 fdc-test /x86_64/fdc/read_without_media
==7804==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 4 fdc-test /x86_64/fdc/media_change
PASS 5 fdc-test /x86_64/fdc/sense_interrupt
PASS 6 fdc-test /x86_64/fdc/relative_seek
---
PASS 11 test-aio /aio/event/wait
PASS 12 test-aio /aio/event/flush
PASS 13 test-aio /aio/event/wait/no-flush-cb
==7812==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 14 test-aio /aio/timer/schedule
PASS 15 test-aio /aio/coroutine/queue-chaining
PASS 16 test-aio /aio-gsource/flush
---
PASS 11 fdc-test /x86_64/fdc/read_no_dma_18
PASS 28 test-aio /aio-gsource/timer/schedule
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-aio-multithread -m=quick -k --tap < /dev/null | 
./scripts/tap-driver.pl --test-name="test-aio-multithread" 
==7818==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 test-aio-multithread /aio/multi/lifecycle
PASS 2 test-aio-multithread /aio/multi/schedule
PASS 3 test-aio-multithread /aio/multi/mutex/contended
---
PASS 4 test-aio-multithread /aio/multi/mutex/handoff
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img 
tests/ide-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="ide-test" 
PASS 5 test-aio-multithread /aio/multi/mutex/mcs
==7852==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 ide-test /x86_64/ide/identify
PASS 6 test-aio-multithread /aio/multi/mutex/pthread
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-throttle -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="test-throttle" 
==7867==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
==7863==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 1 test-throttle /throttle/leak_bucket
PASS 2 test-throttle /throttle/compute_wait
PASS 3 test-throttle /throttle/init
---
PASS 14 test-throttle /throttle/config/max
PASS 15 test-throttle /throttle/config/iops_size
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  
tests/test-thread-pool -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl 
--test-name="test-thread-pool" 
==7876==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 2 ide-test /x86_64/ide/flush
PASS 1 test-thread-pool /thread-pool/submit
PASS 2 test-thread-pool /thread-pool/submit-aio
PASS 3 test-thread-pool /thread-pool/submit-co
PASS 4 test-thread-pool /thread-pool/submit-many
==7943==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 3 ide-test /x86_64/ide/bmdma/simple_rw
==7949==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 5 test-thread-pool /thread-pool/cancel
PASS 4 ide-test /x86_64/ide/bmdma/trim
==7955==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 5 ide-test /x86_64/ide/bmdma/short_prdt
PASS 6 test-thread-pool /thread-pool/cancel-async
==7961==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}  

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Philippe Mathieu-Daudé]

On 6/14/19 11:42 AM, Kevin Wolf wrote:
> Am 13.06.2019 um 21:41 hat Stefan Weil geschrieben:
>> On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
>>> Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
>> [...]
>>> Note, libssh is not available on MinGW.
>>
>> Nor is it available for Mingw64:

Yes, by "MinGW" I mean both 32/64 implementations.

>>
>> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh=x86_64
>>
>> That makes building for Windows more difficult because there is an
>> additional dependency which must be built from source.
> 
> How many people do actually use the ssh block driver on Windows, though?
> Isn't just building QEMU without it a quite reasonable option, too?
> 
> I wouldn't consider this a strong argument why we should keep using an
> obsolete library.

I agree with Kevin. The only user of the 'ssh' block driver that I am
aware of is the virt-v2v tool:

http://libguestfs.org/virt-v2v.1.html#convert-from-esxi-hypervisor-over-ssh-to-local-libvirt

Stefan, do you think someone would use it on a Windows host?

Thanks,

Phil.

Re: [Qemu-block] [PATCH 0/4] block: drop bs->job

[Kevin Wolf]

Am 06.06.2019 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> These series follow Kevin's suggestions under 
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00670.html
> [Qemu-devel] [PATCH v2 0/2] introduce pinned blk
> (hope you don't mind me using exactly your wording in 02)

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH] block/replication.c: Fix crash issue after failover

[Kevin Wolf]

Am 14.06.2019 um 11:28 hat Zhang Chen geschrieben:
> From: Zhang Chen 
> 
> No block job on active disk after failover.
> In the replication_stop() function have canceled the block job,
> we check it again here.
> 
> Signed-off-by: Zhang Chen 
> ---
>  block/replication.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/replication.c b/block/replication.c
> index 3d4dedddfc..bdf2bf4bbc 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -146,7 +146,9 @@ static void replication_close(BlockDriverState *bs)
>  replication_stop(s->rs, false, NULL);
>  }
>  if (s->stage == BLOCK_REPLICATION_FAILOVER) {
> -job_cancel_sync(>active_disk->bs->job->job);
> +if (s->secondary_disk->bs->job) {
> +job_cancel_sync(>secondary_disk->bs->job->job);
> +}

Why are you changing the code from active_disk to secondary_disk?

Also, please rebase on top of Vladimir's '[PATCH 0/4] block: drop
bs->job'.

Kevin

[Qemu-block] [PATCH v2 8/9] tests/docker: Kludge for missing libunistring.so symlink on Ubuntu 18.04

[Philippe Mathieu-Daudé]

When linking statically on Ubuntu 18.04 we get:

  $ make subdir-x86_64-softmmu
  [...]
 LINKx86_64-softmmu/qemu-system-x86_64
  c++: error: /usr/lib/x86_64-linux-gnu/libunistring.so: No such file or 
directory

This library is pulled in by GTK:

  $ pkg-config --libs --static gtk+-3.0
  -lgtk-3 -latk-bridge-2.0 -latspi -ldbus-1 -lpthread -lsystemd -lgdk-3 
-lgio-2.0 -lXinerama -lXi -lXrandr -lXcursor -lXcomposite -lXdamage -lXfixes 
-lxkbcommon -lwayland-cursor -lwayland-egl -lwayland-client -lepoxy -ldl 
-lpangocairo-1.0 -lpangoft2-1.0 -lharfbuzz -lm -lgraphite2 -lpango-1.0 -lm 
-latk-1.0 -lcairo-gobject -lcairo -lz -lpixman-1 -lfontconfig -lexpat 
-lfreetype -lexpat -lfreetype -lpng16 -lm -lz -lm -lxcb-shm -lxcb-render 
-lXrender -lXext -lX11 -lpthread -lxcb -lXau -lXdmcp -lgdk_pixbuf-2.0 -lm 
-lpng16 -lm -lz -lm -lz -lgio-2.0 -lz -lresolv -lselinux -lmount -lgmodule-2.0 
-pthread -ldl -lgobject-2.0 -lffi -lglib-2.0 -pthread -lpcre -pthread

However, while the library is presentm, its symlink is missing:

  $ ls -ld /usr/lib/x86_64-linux-gnu/libunistring.so
  ls: cannot access '/usr/lib/x86_64-linux-gnu/libunistring.so': No such file 
or directory

  $ ls -ld /usr/lib/x86_64-linux-gnu/libunistring.so*
  lrwxrwxrwx. 1 root root  21 Mar  3  2018 
/usr/lib/x86_64-linux-gnu/libunistring.so.2 -> libunistring.so.2.1.0
  -rw-r--r--. 1 root root 1562664 Mar  3  2018 
/usr/lib/x86_64-linux-gnu/libunistring.so.2.1.0

Fix the issue by creating the missing symlink manually.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/docker/dockerfiles/ubuntu1804.docker | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/docker/dockerfiles/ubuntu1804.docker 
b/tests/docker/dockerfiles/ubuntu1804.docker
index 2e2900150b..7e45c52166 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -54,4 +54,8 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
 apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
+# The libunistring2 package does not create a symlink to libunistring.so
+# Create it manually to fix:
+# error: /usr/lib/x86_64-linux-gnu/libunistring.so: No such file or directory
+RUN ln -s libunistring.so.2 /usr/lib/x86_64-linux-gnu/libunistring.so
 ENV FEATURES clang pyyaml sdl2
-- 
2.20.1

[Qemu-block] [PATCH v2 9/9] .travis.yml: Test softmmu static linking

[Philippe Mathieu-Daudé]

Add a test to avoid the ./configure script to bitrot.

Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 08502c0aa2..6962fff826 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -92,6 +92,11 @@ matrix:
 - CONFIG="--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}"
 
 
+# Test static linking
+- env:
+- CONFIG="--static --target-list=lm32-softmmu"
+
+
 # Just build tools and run minimal unit and softfloat checks
 - env:
 - BASE_CONFIG="--enable-tools"
-- 
2.20.1

[Qemu-block] [PATCH v2 4/9] configure: Link test before auto-enabling libusbredir library

[Philippe Mathieu-Daudé]

Similarly to commit a73e82ef912, test the library links correctly
before considering it as usable.

This fixes using ./configure --static on Ubuntu 18.04:

  $ make subdir-aarch64-softmmu
  [...]
LINKaarch64-softmmu/qemu-system-aarch64
  /usr/bin/ld: cannot find -lusbredirparser
  collect2: error: ld returned 1 exit status
  Makefile:204: recipe for target 'qemu-system-aarch64' failed
  make[1]: *** [qemu-system-aarch64] Error 1

  $ fgrep redir config-host.mak
  USB_REDIR_LIBS=-lusbredirparser

  $ lsb_release -cri
  Distributor ID: Ubuntu
  Release:18.04
  Codename:   bionic

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 2ebaa32746..0dd6e8bed3 100755
--- a/configure
+++ b/configure
@@ -4918,9 +4918,19 @@ fi
 # check for usbredirparser for usb network redirection support
 if test "$usb_redir" != "no" ; then
 if $pkg_config --atleast-version=0.6 libusbredirparser-0.5; then
-usb_redir="yes"
 usb_redir_cflags=$($pkg_config --cflags libusbredirparser-0.5)
 usb_redir_libs=$($pkg_config --libs libusbredirparser-0.5)
+# Packaging for the static libraries is not always correct.
+# At least ubuntu 18.04 ships only shared libraries.
+write_c_skeleton
+if ! compile_prog "$usb_redir_cflags" "$usb_redir_libs" ; then
+if test "$usb_redir" = "yes" ; then
+  error_exit "usbredir check failed."
+fi
+usb_redir="no"
+else
+usb_redir="yes"
+fi
 else
 if test "$usb_redir" = "yes"; then
 feature_not_found "usb-redir" "Install usbredir devel"
-- 
2.20.1

[Qemu-block] [PATCH v2 7/9] configure: Link test before auto-enabling GTK libraries

[Philippe Mathieu-Daudé]

Similarly to commit a73e82ef912, test the libraries link correctly
before considering them as usable.

This fixes using ./configure --static on Ubuntu 18.04:

  $ make subdir-lm32-softmmu
  [...]
  LINKlm32-softmmu/qemu-system-lm32
  /usr/bin/ld: cannot find -lgtk-3
  /usr/bin/ld: cannot find -latk-bridge-2.0
  /usr/bin/ld: cannot find -latspi
  /usr/bin/ld: cannot find -lsystemd
  /usr/bin/ld: cannot find -lgdk-3
  /usr/bin/ld: cannot find -lwayland-cursor
  /usr/bin/ld: cannot find -lwayland-egl
  /usr/bin/ld: cannot find -lwayland-client
  /usr/bin/ld: cannot find -lepoxy
  /usr/bin/ld: cannot find -lgraphite2
  collect2: error: ld returned 1 exit status
  Makefile:204: recipe for target 'qemu-system-lm32' failed
  make[1]: *** [qemu-system-lm32] Error 1

  $ fgrep gdk config-host.mak
  GTK_LIBS=-lgtk-3 -latk-bridge-2.0 -latspi -ldbus-1 -lpthread -lsystemd 
-lgdk-3 -lgio-2.0 -lXinerama -lXi -lXrandr -lXcursor -lXcomposite -lXdamage 
-lXfixes -lxkbcommon -lwayland-cursor -lwayland-egl -lwayland-client -lepoxy 
-ldl -lpangocairo-1.0 -lpangoft2-1.0 -lharfbuzz -lm -lgraphite2 -lpango-1.0 -lm 
-latk-1.0 -lcairo-gobject -lcairo -lz -lpixman-1 -lfontconfig -lexpat 
-lfreetype -lexpat -lfreetype -lpng16 -lm -lz -lm -lxcb-shm -lxcb-render 
-lXrender -lXext -lX11 -lpthread -lxcb -lXau -lXdmcp -lgdk_pixbuf-2.0 -lm 
-lpng16 -lm -lz -lm -lz -lgio-2.0 -lz -lresolv -lselinux -lmount -lgmodule-2.0 
-pthread -ldl -lgobject-2.0 -lffi -lglib-2.0 -pthread -lpcre -pthread -lX11 
-lpthread -lxcb -lXau -lXdmcp
VTE_CFLAGS=-pthread -I/usr/include/vte-2.91 -I/usr/include/gtk-3.0 
-I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 
-I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include 
-I/usr/include/gtk-3.0 -I/usr/include/cairo -I/usr/include/pango-1.0 
-I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 
-I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/freetype2 
-I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 
-I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 
-I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/p11-kit-1

  $ lsb_release -cri
  Distributor ID: Ubuntu
  Release:18.04
  Codename:   bionic

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index a3da5adf80..ffd269b34f 100755
--- a/configure
+++ b/configure
@@ -2782,7 +2782,17 @@ if test "$gtk" != "no"; then
 gtk_cflags="$gtk_cflags $x11_cflags"
 gtk_libs="$gtk_libs $x11_libs"
 fi
-gtk="yes"
+# Packaging for the static libraries is not always correct.
+# At least ubuntu 18.04 ships only shared libraries.
+write_c_skeleton
+if ! compile_prog "$gtk_cflags" "$gtk_libs" ; then
+if test "$gtk" = "yes" ; then
+  error_exit "gtk check failed."
+fi
+gtk="no"
+else
+gtk="yes"
+fi
 elif test "$gtk" = "yes"; then
 feature_not_found "gtk" "Install gtk3-devel"
 else
-- 
2.20.1

[Qemu-block] [PATCH v2 6/9] configure: Link test before auto-enabling OpenGL libraries

[Philippe Mathieu-Daudé]

Similarly to commit a73e82ef912, test the libraries link correctly
before considering them as usable.

This fixes using ./configure --static on Ubuntu 18.04:

  $ make subdir-lm32-softmmu
  [...]
  LINKlm32-softmmu/qemu-system-lm32
  /usr/bin/ld: cannot find -lepoxy
  /usr/bin/ld: cannot find -lgbm
  collect2: error: ld returned 1 exit status
  Makefile:204: recipe for target 'qemu-system-lm32' failed
  make[1]: *** [qemu-system-lm32] Error 1

  $ fgrep epoxy config-host.mak
  OPENGL_LIBS=-lepoxy -ldl -lgbm -ldl

  $ lsb_release -cri
  Distributor ID: Ubuntu
  Release:18.04
  Codename:   bionic

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 449dbd69ce..a3da5adf80 100755
--- a/configure
+++ b/configure
@@ -4133,11 +4133,21 @@ if test "$opengl" != "no" ; then
   if $pkg_config $opengl_pkgs; then
 opengl_cflags="$($pkg_config --cflags $opengl_pkgs)"
 opengl_libs="$($pkg_config --libs $opengl_pkgs)"
-opengl=yes
-if test "$gtk" = "yes" && $pkg_config --exists "$gtkpackage >= 3.16"; then
-gtk_gl="yes"
+# Packaging for the static libraries is not always correct.
+# At least ubuntu 18.04 ships only shared libraries.
+write_c_skeleton
+if ! compile_prog "$opengl_cflags" "$opengl_libs" ; then
+if test "$opengl" = "yes" ; then
+  error_exit "opengl check failed."
+fi
+opengl=no
+else
+opengl=yes
+if test "$gtk" = "yes" && $pkg_config --exists "$gtkpackage >= 3.16"; 
then
+gtk_gl="yes"
+fi
+QEMU_CFLAGS="$QEMU_CFLAGS $opengl_cflags"
 fi
-QEMU_CFLAGS="$QEMU_CFLAGS $opengl_cflags"
   else
 if test "$opengl" = "yes" ; then
   feature_not_found "opengl" "Please install opengl (mesa) devel pkgs: 
$opengl_pkgs"
-- 
2.20.1

[Qemu-block] [PATCH v2 5/9] configure: Link test before auto-enabling PulseAudio library

[Philippe Mathieu-Daudé]

Similarly to commit a73e82ef912, test the library links correctly
before considering it as usable.

This fixes using ./configure --static on Ubuntu 18.04:

  $ make subdir-aarch64-softmmu
  [...]
LINKaarch64-softmmu/qemu-system-aarch64
  /usr/bin/ld: cannot find -lpulse
  /usr/bin/ld: cannot find -lpulsecommon-11.1
  collect2: error: ld returned 1 exit status
  Makefile:204: recipe for target 'qemu-system-aarch64' failed
  make[1]: *** [qemu-system-aarch64] Error 1

  $ fgrep pulse config-host.mak
  PULSE_LIBS=-L/usr/lib/aarch64-linux-gnu/pulseaudio -lpulse -lpulsecommon-11.1

  $ lsb_release -cri
  Distributor ID: Ubuntu
  Release:18.04
  Codename:   bionic

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 0dd6e8bed3..449dbd69ce 100755
--- a/configure
+++ b/configure
@@ -3408,10 +3408,21 @@ for drv in $audio_drv_list; do
 
 pa | try-pa)
 if $pkg_config libpulse --exists; then
-pulse_libs=$($pkg_config libpulse --libs)
-audio_pt_int="yes"
-if test "$drv" = "try-pa"; then
-audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/try-pa/pa/')
+pulse_cflags=$($pkg_config --cflags libpulse)
+pulse_libs=$($pkg_config --libs libpulse)
+# Packaging for the static libraries is not always correct.
+# At least ubuntu 18.04 ships only shared libraries.
+write_c_skeleton
+if ! compile_prog "$pulse_cflags" "$pulse_libs" ; then
+unset pulse_cflags pulse_libs
+if test "$drv" = "try-pa"; then
+audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/try-pa//')
+fi
+else
+audio_pt_int="yes"
+if test "$drv" = "try-pa"; then
+audio_drv_list=$(echo "$audio_drv_list" | sed -e 
's/try-pa/pa/')
+fi
 fi
 else
 if test "$drv" = "try-pa"; then
-- 
2.20.1

[Qemu-block] [PATCH v2 3/9] configure: Link test before auto-enabling libusb library

[Philippe Mathieu-Daudé]

Similarly to commit a73e82ef912, test the library links correctly
before considering it as usable.

This fixes using ./configure --static on Ubuntu 18.04:

  $ make subdir-aarch64-softmmu
  [...]
LINKaarch64-softmmu/qemu-system-aarch64
  /usr/bin/ld: cannot find -ludev
  collect2: error: ld returned 1 exit status
  Makefile:204: recipe for target 'qemu-system-aarch64' failed
  make[1]: *** [qemu-system-aarch64] Error 1

  $ fgrep udev config-host.mak
  LIBUSB_LIBS=-lusb-1.0 -ludev -pthread

  $ lsb_release -cri
  Distributor ID: Ubuntu
  Release:18.04
  Codename:   bionic

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index fe0e2e1b75..2ebaa32746 100755
--- a/configure
+++ b/configure
@@ -4894,9 +4894,19 @@ fi
 # check for libusb
 if test "$libusb" != "no" ; then
 if $pkg_config --atleast-version=1.0.13 libusb-1.0; then
-libusb="yes"
 libusb_cflags=$($pkg_config --cflags libusb-1.0)
 libusb_libs=$($pkg_config --libs libusb-1.0)
+# Packaging for the static libraries is not always correct.
+# At least ubuntu 18.04 ships only shared libraries.
+write_c_skeleton
+if ! compile_prog "$libusb_cflags" "$libusb_libs" ; then
+if test "$libusb" = "yes" ; then
+  error_exit "libusb check failed."
+fi
+libusb="no"
+else
+libusb="yes"
+fi
 else
 if test "$libusb" = "yes"; then
 feature_not_found "libusb" "Install libusb devel >= 1.0.13"
-- 
2.20.1

[Qemu-block] [PATCH v2 1/9] configure: Only generate GLUSTERFS variables if glusterfs is usable

[Philippe Mathieu-Daudé]

It is pointless and confusing to have GLUSTERFS variables
in config-host.mak when glusterfs is not usable.

Reviewed-by: Niels de Vos 
Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/configure b/configure
index b091b82cb3..13fd4a1166 100755
--- a/configure
+++ b/configure
@@ -7118,30 +7118,30 @@ if test "$glusterfs" = "yes" ; then
   echo "CONFIG_GLUSTERFS=m" >> $config_host_mak
   echo "GLUSTERFS_CFLAGS=$glusterfs_cflags" >> $config_host_mak
   echo "GLUSTERFS_LIBS=$glusterfs_libs" >> $config_host_mak
-fi
 
-if test "$glusterfs_xlator_opt" = "yes" ; then
-  echo "CONFIG_GLUSTERFS_XLATOR_OPT=y" >> $config_host_mak
-fi
+  if test "$glusterfs_xlator_opt" = "yes" ; then
+echo "CONFIG_GLUSTERFS_XLATOR_OPT=y" >> $config_host_mak
+  fi
 
-if test "$glusterfs_discard" = "yes" ; then
-  echo "CONFIG_GLUSTERFS_DISCARD=y" >> $config_host_mak
-fi
+  if test "$glusterfs_discard" = "yes" ; then
+echo "CONFIG_GLUSTERFS_DISCARD=y" >> $config_host_mak
+  fi
 
-if test "$glusterfs_fallocate" = "yes" ; then
-  echo "CONFIG_GLUSTERFS_FALLOCATE=y" >> $config_host_mak
-fi
+  if test "$glusterfs_fallocate" = "yes" ; then
+echo "CONFIG_GLUSTERFS_FALLOCATE=y" >> $config_host_mak
+  fi
 
-if test "$glusterfs_zerofill" = "yes" ; then
-  echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
-fi
+  if test "$glusterfs_zerofill" = "yes" ; then
+echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
+  fi
 
-if test "$glusterfs_ftruncate_has_stat" = "yes" ; then
-  echo "CONFIG_GLUSTERFS_FTRUNCATE_HAS_STAT=y" >> $config_host_mak
-fi
+  if test "$glusterfs_ftruncate_has_stat" = "yes" ; then
+echo "CONFIG_GLUSTERFS_FTRUNCATE_HAS_STAT=y" >> $config_host_mak
+  fi
 
-if test "$glusterfs_iocb_has_stat" = "yes" ; then
-  echo "CONFIG_GLUSTERFS_IOCB_HAS_STAT=y" >> $config_host_mak
+  if test "$glusterfs_iocb_has_stat" = "yes" ; then
+echo "CONFIG_GLUSTERFS_IOCB_HAS_STAT=y" >> $config_host_mak
+  fi
 fi
 
 if test "$libssh2" = "yes" ; then
-- 
2.20.1

[Qemu-block] [PATCH v2 0/9] configure: Fix softmmu --static linking

[Philippe Mathieu-Daudé]

Hi,

Apparently QEMU static linking is slowly bitroting. Obviously it
depends the libraries an user has installed, anyway it seems there
are not much testing done.

This series fixes few issues, enough to build QEMU on a Ubuntu
18.04 host.

Peter commented on v1:

  The main reason for supporting static linking is so we can build
  the user-mode emulators. Almost always the problems with
  static linking the softmmu binaries and the tools are
  issues with the distro's packaging of the static libraries
  (pkg-config files which specify things that don't work for
  static is a common one).

  So we could put in a lot of checking of "is what pkg-config
  tells us broken". Or we could just say "we don't support static
  linking for anything except the usermode binaries". We
  should probably phase in deprecation of that because it's
  possible somebody's using it seriously, but it seems like
  a fairly weird thing to do to me.

I share his view on this (restricting static linking to qemu-user)
but since the work was already done when I read his comment, I still
send the v2.

Since v1:
- pkg-config already use the '--static' argument, do not add it twice
- Fixed x86_64 host builds (was missing GTK and OpenGL patches)
- Added Niels R-b tag on the first patch
- The Travis-CI job now succeeds:
  https://travis-ci.org/philmd/qemu/jobs/545653697 (6 min 7 sec)

Regards,

Phil.

Philippe Mathieu-Daudé (9):
  configure: Only generate GLUSTERFS variables if glusterfs is usable
  configure: Link test before auto-enabling GlusterFS libraries
  configure: Link test before auto-enabling libusb library
  configure: Link test before auto-enabling libusbredir library
  configure: Link test before auto-enabling PulseAudio library
  configure: Link test before auto-enabling OpenGL libraries
  configure: Link test before auto-enabling GTK libraries
  tests/docker: Kludge for missing libunistring.so symlink on Ubuntu
18.04
  .travis.yml: Test softmmu static linking

 .travis.yml|   5 +
 configure  | 121 -
 tests/docker/dockerfiles/ubuntu1804.docker |   4 +
 3 files changed, 100 insertions(+), 30 deletions(-)

-- 
2.20.1

Re: [Qemu-block] [PATCH] iotests: Hide timestamps for skipped tests

[Kevin Wolf]

Am 13.06.2019 um 20:37 hat Max Reitz geschrieben:
> Currently, the "thistime" variable is not reinitialized on every loop
> iteration.  This leads to tests that do not yield a run time (because
> they failed or were skipped) printing the run time of the previous test
> that did.  Fix that by reinitializing "thistime" for every test.
> 
> Signed-off-by: Max Reitz 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [QEMU] [PATCH v2 0/8] Add Qemu to SeaBIOS LCHS interface

[Sam Eiderman]

> On 14 Jun 2019, at 7:43, Gerd Hoffmann  wrote:
> 
>  Hi,
> 
>> Can there be a guest that will fail the MBR in such a way? Yes.
>> Look at the following MBR partition table of a Windows XP guest in our 
>> production
>> environment:
>> 
>> Disk size in sectors: 16777216
>> 
>> Binary (only one partition 16 bytes): 80 01 01 00 07 fe ff ff 3f 00 00 00 d5 
>> ea ff 00
>> Start: (0, 1, 1, 63)
>> End: (1023, 254, 63, 16771859)
>> 
>> As can be easily seen, any MBR guessing algorithm should guess:
>> 
>>  255 heads (since a value of 254 appears), 63 spt (since a value of 63 
>> appears)
>> 
>> Turns out that this image does not work with 255, 63 but actually requires
>> 
>>  16 heads, 63 spt
>> 
>> to boot.
>> 
>> So relying on MBR partitions alone is not always enough and sometimes manual 
>> intervention
>> is required.
> 
> Ok, given that seabios has no setup any manual configuration needs to be done 
> via qemu.
> 
> But why do we need a new interface for that?  IDE can pass the geometry
> to the guest.  virtio-blk has support too (VIRTIO_BLK_F_GEOMETRY).
> Likewise scsi (MODE_PAGE_HD_GEOMETRY).  So this should be doable without
> any qemu changes.

This was indeed considered (all 3 methods) but it has the following issues:

Physical geometries of devices must now also be logical geometries with 
translation=none.
When the OS will query these devices - It will now see different physical 
geometries, adapted to be logical geometries.
I’m not sure even how to implement this without breaking existing compatibility 
- since we don’t want to affect logical geometries of currently used guests.
MODE_PAGE_HD_GEOMETRY does not contain the spts, only cylinders (as 3 byte 
number) and heads (as 1 byte number) and computes the spts using: 
number_of_total_sectors / (heads * cylinders), this means that qemu now must 
report number_of_total_sectors as heads * cylinders * spt for SeaBIOS to 
correctly receive the number of spts - this is not optimal since 
number_of_total_sectors can not reflect the real amount of sectors in the disk 
which should be reported from CDB_CMD_READ_CAPACITY.
Moving a scsi-hd/virtio-blk with 255 physical heads to ide-hd, we will still 
need to report 255 heads - this is possible since a whole byte can be used in 
the “ide identify” command, but goes against the spec of a maximum of 16 heads 
for IDE.

Overall this approach is much more complicated.

Sam

> 
> cheers,
>  Gerd
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Vladimir Sementsov-Ogievskiy]

13.06.2019 19:03, Max Reitz wrote:
> [re-adding the original CCs, why not]
> 
> On 13.06.19 16:30, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 17:21, Max Reitz wrote:
>>> On 13.06.19 16:19, Vladimir Sementsov-Ogievskiy wrote:
 13.06.2019 1:08, Max Reitz wrote:
> If the main loop cancels all block jobs while the block layer is not
> drained, this cancelling may not happen instantaneously.  We can start a
> drained section before vm_shutdown(), which entails another
> bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
> basically.
>
> We do not have to end the drained section, because we actually do not
> want any requests to happen from this point on.
>
> Signed-off-by: Max Reitz 
> ---
> I don't know whether it actually makes sense to never end this drained
> section.  It makes sense to me.  Please correct me if I'm wrong.
> ---
> vl.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index cd1fbc4cdc..3f8b3f74f5 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
>  */
> migration_shutdown();
> 
> +/*
> + * We must cancel all block jobs while the block layer is drained,
> + * or cancelling will be affected by throttling and thus may block
> + * for an extended period of time.
> + * vm_shutdown() will bdrv_drain_all(), so we may as well include
> + * it in the drained section.
> + * We do not need to end this section, because we do not want any
> + * requests happening from here on anyway.
> + */
> +bdrv_drain_all_begin();
> +
> /* No more vcpu or device emulation activity beyond this point */
> vm_shutdown();
> 
>

 So, actually, the problem is that we may wait for job requests twice:
 on drain and then on cancel.
>>>
>>> We don’t wait on drain.  When the throttle node is drained, it will
>>> ignore throttling (as noted in the cover letter).
>>>
>>> We do wait when cancelling a job while the throttle node isn’t drained,
>>> though.  That’s the problem.
>>
>> Ah, understand now.
>>
>> Is it safe to drain_begin before stopping cpus? We may finish up then with 
>> some queued
>> somewhere IO requests..
> 
> Hm...  Aren’t guest devices prohibited from issuing requests to the
> block layer while their respective block device is drained?

It's at least a buggy place, I remember Denis Plotnikov sent patch to fix it 
and had a huge
discussion with Kevin.
And here it is:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00732.html

> 
> Otherwise, I suppose I’ll have to move the bdrv_drain_all_begin() below
> the vm_shutdown().  That wouldn’t be too big of a problem.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh

[Kevin Wolf]

Am 13.06.2019 um 21:41 hat Stefan Weil geschrieben:
> On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
> > Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
> [...]
> > Note, libssh is not available on MinGW.
> 
> Nor is it available for Mingw64:
> 
> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh=x86_64
> 
> That makes building for Windows more difficult because there is an
> additional dependency which must be built from source.

How many people do actually use the ssh block driver on Windows, though?
Isn't just building QEMU without it a quite reasonable option, too?

I wouldn't consider this a strong argument why we should keep using an
obsolete library.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc

[Kevin Wolf]

Am 14.06.2019 um 11:06 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > monitor.c mixes a lot of different things in a single file: The core
> > monitor infrastructure, HMP infrastrcture, QMP infrastructure, and the
> > implementation of several HMP and QMP commands. Almost worse, struct
> > Monitor mixes state for HMP, for QMP, and state actually shared between
> > all monitors. monitor.c must be linked with a system emulator and even
> > requires per-target compilation because some of the commands it
> > implements access system emulator state.
> >
> > The reason why I care about this is that I'm working on a protoype for a
> > storage daemon, which wants to use QMP (but probably not HMP) and
> > obviously doesn't have any system emulator state. So I'm interested in
> > some core monitor parts that can be linked to non-system-emulator tools.
> >
> > This series first creates separate structs MonitorQMP and MonitorHMP
> > which inherit from Monitor, and then moves the associated infrastructure
> > code into separate source files.
> >
> > While the split is probably not perfect, I think it's an improvement of
> > the current state even for QEMU proper, and it's good enough so I can
> > link my storage daemon against just monitor/core.o and monitor/qmp.o and
> > get a useless QMP monitor that parses the JSON input and rejects
> > everything as an unknown command.
> >
> > Next I'll try to teach it a subset of QMP commands that can actually be
> > supported in a tool, but while there will be a few follow-up patches to
> > achieve this, I don't expect that this work will bring up much that
> > needs to be changed in the splitting process done in this series.
> 
> I think I can address the remaining rather minor issues without a
> respin.  Please let me know if you disagree with any of my remarks.

Feel free to make the changes you suggested, possibly with the exception
of the #includes in monitor-internal.h where I think you're only
partially right (see my reply there).

Please also consider fixing the commit message typo I pointed out for
patch 15.

Kevin

Re: [Qemu-block] [PATCH v8 7/7] block/backup: use backup-top instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

13.06.2019 21:02, Max Reitz wrote:
> On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
>> Drop write notifiers and use filter node instead. Changes:
>>
>> 1. copy-before-writes now handled by filter node, so, drop all
>> is_write_notifier arguments.
>>
>> 2. we don't have intersecting requests, so their handling is dropped.
>> Instead, synchronization works as follows:
>> when backup or backup-top starts copying of some area it firstly
>> clears copy-bitmap bits, and nobody touches areas, not marked with
>> dirty bits in copy-bitmap, so there is no intersection. Also, backup
>> job copy operations are surrounded by bdrv region lock, which is
>> actually serializing request, to not interfere with guest writes and
>> not read changed data from source (before reading we clear
>> corresponding bit in copy-bitmap, so, this area is not more handled by
>> backup-top).
>>
>> 3. To sync with in-flight requests at job finish we now have drained
>> removing of the filter, we don't need rw-lock.
>>
>> == RFC part ==
>>
>> iotests changed:
>> 56: op-blocker doesn't shot now, as we set it on source, but then check
>> on filter, when trying to start second backup... Should I workaround it
>> somehow?
> 
> Hm.  Where does that error message even come from?  The fact that the
> target image is in use already (Due to file locks)?
> 
> It appears that way indeed.
> 
> It seems reasonable to me that you can now run a backup on top of
> another backup.  Well, I mean, it is a stupid thing to do, but I don’t
> see why the block layer would forbid doing so.
> 
> So the test seems superfluous to me.  If we want to keep it (why not),
> it should test the opposite, namely that a backup to a different image
> (with a different job ID) works.  (It seems simple enough to modify the
> job that way, so why not.)
> 
>> 129: Hmm, now it is not busy at this moment.. But it's illegal to check
>> busy, as job has pause-points and set busy to false in these points.
>> Why we assert it in this test?
> 
> Nobody knows, it’s probably wrong.  All I know is that 129 is just
> broken anyway.
> 
>> 141: Obvious, as drv0 is not root node now, but backing of the filter,
>> when we try to remove it.
> 
> I get a failed assertion in 256.  That is probably because the
> bdrv_set_aio_context() calls weren’t as unnecessary as I deemed them to be.

hmm, will check.

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/backup.c | 171 ++---
>>   tests/qemu-iotests/056 |   2 +-
>>   tests/qemu-iotests/129 |   1 -
>>   tests/qemu-iotests/141.out |   2 +-
>>   4 files changed, 68 insertions(+), 108 deletions(-)
> 
> For some reason, my gcc starts to complain that backup_loop() may not
> initialize error_is_read after this patch.  I don’t know why that is.
> Perhaps it inlines backup_do_cow() now?  (So before it just saw that a
> pointer to error_is_read was passed to backup_do_cow() and took it as an
> opaque function, so it surely would set this value somewhere.  Now it
> inlines it and it can’t find whether that will definitely happen, so it
> complains.)
> 
> I don’t think it is strictly necessary to initialize error_is_read, but,
> well, it won’t hurt.
> 
>> diff --git a/block/backup.c b/block/backup.c
>> index 00f4f8af53..a5b8e04c9c 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
> 
> [...]
> 
>> @@ -60,56 +53,17 @@ typedef struct BackupBlockJob {
>>   
>>   static const BlockJobDriver backup_job_driver;
>>   
>> -/* See if in-flight requests overlap and wait for them to complete */
>> -static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
>> -   int64_t start,
>> -   int64_t end)
>> -{
>> -CowRequest *req;
>> -bool retry;
>> -
>> -do {
>> -retry = false;
>> -QLIST_FOREACH(req, >inflight_reqs, list) {
>> -if (end > req->start_byte && start < req->end_byte) {
>> -qemu_co_queue_wait(>wait_queue, NULL);
>> -retry = true;
>> -break;
>> -}
>> -}
>> -} while (retry);
>> -}
>> -
>> -/* Keep track of an in-flight request */
>> -static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
>> -  int64_t start, int64_t end)
>> -{
>> -req->start_byte = start;
>> -req->end_byte = end;
>> -qemu_co_queue_init(>wait_queue);
>> -QLIST_INSERT_HEAD(>inflight_reqs, req, list);
>> -}
>> -
>> -/* Forget about a completed request */
>> -static void cow_request_end(CowRequest *req)
>> -{
>> -QLIST_REMOVE(req, list);
>> -qemu_co_queue_restart_all(>wait_queue);
>> -}
>> -
>>   /* Copy range to target with a bounce buffer and return the bytes copied. 
>> If
>>* error occurred, return a negative error number */
>>   static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>

Re: [Qemu-block] [Qemu-devel] [PATCH v3 11/15] monitor: Split out monitor/hmp.c

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Kevin Wolf  writes:
> 
> > Move HMP infrastructure from monitor/misc.c to monitor/hmp.c. This is
> > code that can be shared for all targets, so compile it only once.
> >
> > The amount of function and particularly extern variables in
> > monitor_int.h is probably a bit larger than it needs to be, but this way
> > no non-trivial code modifications are needed. The interfaces between HMP
> > and the monitor core can be cleaned up later.
> >
> > Signed-off-by: Kevin Wolf 
> > ---
> [...]
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > new file mode 100644
> > index 00..3621b195ed
> > --- /dev/null
> > +++ b/monitor/hmp.c
> > @@ -0,0 +1,1415 @@
> [...]
> > +static int64_t expr_unary(Monitor *mon)
> > +{
> > +int64_t n;
> > +char *p;
> > +int ret;
> > +
> > +switch (*pch) {
> > +case '+':
> > +next();
> > +n = expr_unary(mon);
> > +break;
> > +case '-':
> > +next();
> > +n = -expr_unary(mon);
> > +break;
> > +case '~':
> > +next();
> > +n = ~expr_unary(mon);
> > +break;
> > +case '(':
> > +next();
> > +n = expr_sum(mon);
> > +if (*pch != ')') {
> > +expr_error(mon, "')' expected");
> > +}
> > +next();
> > +break;
> > +case '\'':
> > +pch++;
> > +if (*pch == '\0') {
> > +expr_error(mon, "character constant expected");
> > +}
> > +n = *pch;
> > +pch++;
> > +if (*pch != '\'') {
> > +expr_error(mon, "missing terminating \' character");
> > +}
> > +next();
> > +break;
> > +case '$':
> > +{
> > +char buf[128], *q;
> > +int64_t reg = 0;
> > +
> > +pch++;
> > +q = buf;
> > +while ((*pch >= 'a' && *pch <= 'z') ||
> > +   (*pch >= 'A' && *pch <= 'Z') ||
> > +   (*pch >= '0' && *pch <= '9') ||
> > +   *pch == '_' || *pch == '.') {
> > +if ((q - buf) < sizeof(buf) - 1) {
> > +*q++ = *pch;
> > +}
> > +pch++;
> > +}
> > +while (qemu_isspace(*pch)) {
> > +pch++;
> > +}
> > +*q = 0;
> > +ret = get_monitor_def(, buf);
> > +if (ret < 0) {
> > +expr_error(mon, "unknown register");
> > +}
> > +n = reg;
> > +}
> > +break;
> > +case '\0':
> > +expr_error(mon, "unexpected end of expression");
> > +n = 0;
> > +break;
> > +default:
> > +errno = 0;
> > +n = strtoull(pch, , 0);
> 
> checkpatch.pl gripes:
> 
> ERROR: consider using qemu_strtoull in preference to strtoull
> 
> 
> Let's add a TODO comment.  
> 
> > +if (errno == ERANGE) {
> > +expr_error(mon, "number too large");
> > +}
> > +if (pch == p) {
> > +expr_error(mon, "invalid char '%c' in expression", *p);
> > +}
> > +pch = p;
> > +while (qemu_isspace(*pch)) {
> > +pch++;
> > +}
> > +break;
> > +}
> > +return n;
> > +}
> [...]
> > +static void monitor_find_completion(void *opaque,
> > +const char *cmdline)
> > +{
> > +MonitorHMP *mon = opaque;
> > +char *args[MAX_ARGS];
> > +int nb_args, len;
> > +
> > +/* 1. parse the cmdline */
> > +if (parse_cmdline(cmdline, _args, args) < 0) {
> > +return;
> > +}
> > +
> > +/* if the line ends with a space, it means we want to complete the
> > + * next arg */
> 
> checkpatch.pl again:
> 
> WARNING: Block comments use a leading /* on a separate line
> WARNING: Block comments use a trailing */ on a separate line
> 
> Can touch up in my tree.

I wouldn't worry too much about fixing the existing problems here -
let's get the reorg done through kwolf's patches and then it's easier
to clean up later.

Dave

> > +len = strlen(cmdline);
> > +if (len > 0 && qemu_isspace(cmdline[len - 1])) {
> > +if (nb_args >= MAX_ARGS) {
> > +goto cleanup;
> > +}
> > +args[nb_args++] = g_strdup("");
> > +}
> > +
> > +/* 2. auto complete according to args */
> > +monitor_find_completion_by_table(mon, hmp_cmds, args, nb_args);
> > +
> > +cleanup:
> > +free_cmdline_args(args, nb_args);
> > +}
> [...]
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index 368b8297d4..c8289959c0 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> [...]
> > @@ -612,245 +580,27 @@ out:
> >  return output;
> >  }
> >  
> > -static int compare_cmd(const char *name, const char *list)
> > +/**
> > + * Is @name in the '|' separated list of names @list?
> > + */
> > +int hmp_compare_cmd(const char *name, const char *list)
> >  {
> >  

Re: [Qemu-block] [Qemu-devel] [PATCH v3 15/15] vl: Deprecate -mon pretty=... for HMP monitors

[Kevin Wolf]

Am 14.06.2019 um 11:01 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > The -mon pretty=on|off switch of the -mon option applies only the QMP

s/the QMP/to QMP/

Can you fix up this one as well while you're at it?

> > monitors. It used to be silently ignored for HMP. Deprecate this
> > combination so that we can make it an error in future versions.
> >
> > Signed-off-by: Kevin Wolf 

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc

[Markus Armbruster]

Kevin Wolf  writes:

> monitor.c mixes a lot of different things in a single file: The core
> monitor infrastructure, HMP infrastrcture, QMP infrastructure, and the
> implementation of several HMP and QMP commands. Almost worse, struct
> Monitor mixes state for HMP, for QMP, and state actually shared between
> all monitors. monitor.c must be linked with a system emulator and even
> requires per-target compilation because some of the commands it
> implements access system emulator state.
>
> The reason why I care about this is that I'm working on a protoype for a
> storage daemon, which wants to use QMP (but probably not HMP) and
> obviously doesn't have any system emulator state. So I'm interested in
> some core monitor parts that can be linked to non-system-emulator tools.
>
> This series first creates separate structs MonitorQMP and MonitorHMP
> which inherit from Monitor, and then moves the associated infrastructure
> code into separate source files.
>
> While the split is probably not perfect, I think it's an improvement of
> the current state even for QEMU proper, and it's good enough so I can
> link my storage daemon against just monitor/core.o and monitor/qmp.o and
> get a useless QMP monitor that parses the JSON input and rejects
> everything as an unknown command.
>
> Next I'll try to teach it a subset of QMP commands that can actually be
> supported in a tool, but while there will be a few follow-up patches to
> achieve this, I don't expect that this work will bring up much that
> needs to be changed in the splitting process done in this series.

I think I can address the remaining rather minor issues without a
respin.  Please let me know if you disagree with any of my remarks.

Thanks for helping out with the monitor code!  I know it's rather crusty
in places.

Dave, I'll take this through my tree, if you don't mind.

Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy]

13.06.2019 18:57, Max Reitz wrote:
> On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
>> Backup-top filter does copy-before-write operation. It should be
>> inserted above active disk and has a target node for CBW, like the
>> following:
>>
>>  +---+
>>  | Guest |
>>  +---+
>>  |r,w
>>  v
>>  ++  target   +---+
>>  | backup_top |-->| target(qcow2) |
>>  ++   CBW +---+
>>  |
>> backing |r,w
>>  v
>>  +-+
>>  | Active disk |
>>  +-+
>>
>> The driver will be used in backup instead of write-notifiers.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/backup-top.h  |  64 +
>>   block/backup-top.c  | 322 
>>   block/Makefile.objs |   2 +
>>   3 files changed, 388 insertions(+)
>>   create mode 100644 block/backup-top.h
>>   create mode 100644 block/backup-top.c
>>
>> diff --git a/block/backup-top.h b/block/backup-top.h
>> new file mode 100644
>> index 00..788e18c358
>> --- /dev/null
>> +++ b/block/backup-top.h
>> @@ -0,0 +1,64 @@
>> +/*
>> + * backup-top filter driver
>> + *
>> + * The driver performs Copy-Before-Write (CBW) operation: it is injected 
>> above
>> + * some node, and before each write it copies _old_ data to the target node.
>> + *
>> + * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
>> + *
>> + * Author:
>> + *  Sementsov-Ogievskiy Vladimir 
>> + *
>> + * 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 .
>> + */
>> +
>> +#ifndef BACKUP_TOP_H
>> +#define BACKUP_TOP_H
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "block/block_int.h"
>> +
>> +typedef void (*BackupTopProgressCallback)(uint64_t done, void *opaque);
>> +typedef struct BDRVBackupTopState {
>> +HBitmap *copy_bitmap; /* what should be copied to @target on guest 
>> write. */
>> +BdrvChild *target;
>> +
>> +BackupTopProgressCallback progress_cb;
>> +void *progress_opaque;
>> +} BDRVBackupTopState;
>> +
>> +/*
>> + * bdrv_backup_top_append
>> + *
>> + * Append backup_top filter node above @source node. @target node will 
>> receive
>> + * the data backed up during CBE operations. New filter together with 
>> @target
>> + * node are attached to @source aio context.
>> + *
>> + * The resulting filter node is implicit.
> 
> Why?  It’s just as easy for the caller to just make it implicit if it
> should be.  (And usually the caller should decide that.)

Personally, I don't know what are the reasons for filters to bi implicit or not,
I just made it like other job-filters.. I can move making-implicit to the caller
or drop it at all (if it will work).

> 
>> + *
>> + * @copy_bitmap selects regions which needs CBW. Furthermore, backup_top 
>> will
>> + * use exactly this bitmap, so it may be used to control backup_top behavior
>> + * dynamically. Caller should not release @copy_bitmap during life-time of
>> + * backup_top. Progress is tracked by calling @progress_cb function.
>> + */
>> +BlockDriverState *bdrv_backup_top_append(
>> +BlockDriverState *source, BlockDriverState *target,
>> +HBitmap *copy_bitmap, Error **errp);
>> +void bdrv_backup_top_set_progress_callback(
>> +BlockDriverState *bs, BackupTopProgressCallback progress_cb,
>> +void *progress_opaque);
>> +void bdrv_backup_top_drop(BlockDriverState *bs);
>> +
>> +#endif /* BACKUP_TOP_H */
>> diff --git a/block/backup-top.c b/block/backup-top.c
>> new file mode 100644
>> index 00..1daa02f539
>> --- /dev/null
>> +++ b/block/backup-top.c
>> @@ -0,0 +1,322 @@
>> +/*
>> + * backup-top filter driver
>> + *
>> + * The driver performs Copy-Before-Write (CBW) operation: it is injected 
>> above
>> + * some node, and before each write it copies _old_ data to the target node.
>> + *
>> + * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
>> + *
>> + * Author:
>> + *  Sementsov-Ogievskiy Vladimir 
>> + *
>> + * 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 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 15/15] vl: Deprecate -mon pretty=... for HMP monitors

[Markus Armbruster]

Kevin Wolf  writes:

> The -mon pretty=on|off switch of the -mon option applies only the QMP
> monitors. It used to be silently ignored for HMP. Deprecate this
> combination so that we can make it an error in future versions.
>
> Signed-off-by: Kevin Wolf 
> ---
>  vl.c | 10 +-
>  qemu-deprecated.texi |  6 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index 32daa434eb..99a56b5556 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2317,6 +2317,10 @@ static int mon_init_func(void *opaque, QemuOpts *opts, 
> Error **errp)
>  return -1;
>  }
>  
> +if (!qmp && qemu_opt_get(opts, "pretty")) {
> +warn_report("'pretty' is deprecated for HMP monitors, it has no 
> effect "
> +"and will be removed in future versions");
> +}
>  if (qemu_opt_get_bool(opts, "pretty", 0)) {
>  pretty = true;
>  }
> @@ -2362,7 +2366,11 @@ static void monitor_parse(const char *optarg, const 
> char *mode, bool pretty)
>  opts = qemu_opts_create(qemu_find_opts("mon"), label, 1, _fatal);
>  qemu_opt_set(opts, "mode", mode, _abort);
>  qemu_opt_set(opts, "chardev", label, _abort);
> -qemu_opt_set_bool(opts, "pretty", pretty, _abort);
> +if (!strcmp(mode, "control")) {
> +qemu_opt_set_bool(opts, "pretty", pretty, _abort);
> +} else {
> +assert(pretty == false);
> +}
>  monitor_device_index++;
>  }
>  
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 50292d820b..df04f2840b 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -72,6 +72,12 @@ backend settings instead of environment variables.  To 
> ease migration to
>  the new format, the ``-audiodev-help'' option can be used to convert
>  the current values of the environment variables to ``-audiodev'' options.
>  
> +@subsection -mon ...,control=readline,pretty=on|off (since 4.1)
> +
> +The @code{pretty=on|off} switch has no effect for HMP monitors, but is
> +silently ignored. Using the switch with HMP monitors will become an
> +error in the future.
> +
>  @subsection -realtime (since 4.1)
>  
>  The @code{-realtime mlock=on|off} argument has been replaced by the

Good move.

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 03/15] monitor: Make MonitorQMP a child class of Monitor

[Markus Armbruster]

Kevin Wolf  writes:

> Currently, struct Monitor mixes state that is only relevant for HMP,
> state that is only relevant for QMP, and some actually shared state.
> In particular, a MonitorQMP field is present in the state of any
> monitor, even if it's not a QMP monitor and therefore doesn't use the
> state.
>
> As a first step towards a clean separation between QMP and HMP, let
> MonitorQMP extend Monitor and create a MonitorQMP object only when the
> monitor is actually a QMP monitor.
>
> Some places accessed Monitor.qmp unconditionally, even for HMP monitors.
> They can't keep doing this now, so during the conversion, they are
> either changed to become conditional on monitor_is_qmp() or to assert()
> that they always get a QMP monitor.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Dr. David Alan Gilbert 
> ---
>  monitor.c | 220 ++
>  1 file changed, 124 insertions(+), 96 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index a70c1283b1..855a147723 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -168,26 +168,6 @@ struct MonFdset {
[...]
> @@ -712,29 +717,33 @@ static void monitor_data_init(Monitor *mon, int flags, 
> bool skip_flush,
>  }
>  memset(mon, 0, sizeof(Monitor));
>  qemu_mutex_init(>mon_lock);
> -qemu_mutex_init(>qmp.qmp_queue_lock);
>  mon->outbuf = qstring_new();
>  /* Use *mon_cmds by default. */
>  mon->cmd_table = mon_cmds;
>  mon->skip_flush = skip_flush;
>  mon->use_io_thread = use_io_thread;
> -mon->qmp.qmp_requests = g_queue_new();
>  mon->flags = flags;
>  }
>  
> +static void monitor_data_destroy_qmp(MonitorQMP *mon)
> +{
> +json_message_parser_destroy(>parser);
> +qemu_mutex_destroy(>qmp_queue_lock);
> +monitor_qmp_cleanup_req_queue_locked(mon);
> +g_queue_free(mon->qmp_requests);
> +}
> +
>  static void monitor_data_destroy(Monitor *mon)
>  {
>  g_free(mon->mon_cpu_path);
>  qemu_chr_fe_deinit(>chr, false);
>  if (monitor_is_qmp(mon)) {
> -json_message_parser_destroy(>qmp.parser);
> +MonitorQMP *qmp_mon = container_of(mon, MonitorQMP, common);
> +monitor_data_destroy_qmp(qmp_mon);

I dislike declarations hiding among statements, and variables used just
once.  If the variable was used more than once, or its use was in a
complicated expression, or a lengthy line, I'd ask for a blank line to
separate declaration from statement.  But here, I'd prefer

   monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));

Can touch up in my tree.

>  }
>  readline_free(mon->rs);
>  qobject_unref(mon->outbuf);
>  qemu_mutex_destroy(>mon_lock);
> -qemu_mutex_destroy(>qmp.qmp_queue_lock);
> -monitor_qmp_cleanup_req_queue_locked(mon);
> -g_queue_free(mon->qmp.qmp_requests);
>  }
>  
>  char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> @@ -1087,8 +1096,12 @@ static void query_commands_cb(QmpCommand *cmd, void 
> *opaque)
>  CommandInfoList *qmp_query_commands(Error **errp)
>  {
>  CommandInfoList *list = NULL;
> +MonitorQMP *mon;
> +
> +assert(monitor_is_qmp(cur_mon));

Could use monitor_cur_is_qmp().  If I mess with it in my tree anyway,
I'll consider changing to it.

> +mon = container_of(cur_mon, MonitorQMP, common);
>  
> -qmp_for_each_command(cur_mon->qmp.commands, query_commands_cb, );
> +qmp_for_each_command(mon->commands, query_commands_cb, );
>  
>  return list;
>  }
> @@ -1155,16 +1168,16 @@ static void monitor_init_qmp_commands(void)
>   qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
>  }
>  
> -static bool qmp_oob_enabled(Monitor *mon)
> +static bool qmp_oob_enabled(MonitorQMP *mon)
>  {
> -return mon->qmp.capab[QMP_CAPABILITY_OOB];
> +return mon->capab[QMP_CAPABILITY_OOB];
>  }
>  
> -static void monitor_qmp_caps_reset(Monitor *mon)
> +static void monitor_qmp_caps_reset(MonitorQMP *mon)
>  {
> -memset(mon->qmp.capab_offered, 0, sizeof(mon->qmp.capab_offered));
> -memset(mon->qmp.capab, 0, sizeof(mon->qmp.capab));
> -mon->qmp.capab_offered[QMP_CAPABILITY_OOB] = mon->use_io_thread;
> +memset(mon->capab_offered, 0, sizeof(mon->capab_offered));
> +memset(mon->capab, 0, sizeof(mon->capab));
> +mon->capab_offered[QMP_CAPABILITY_OOB] = mon->common.use_io_thread;
>  }
>  
>  /*
> @@ -1172,7 +1185,7 @@ static void monitor_qmp_caps_reset(Monitor *mon)
>   * On success, set mon->qmp.capab[], and return true.
>   * On error, set @errp, and return false.
>   */
> -static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list,
> +static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
>  Error **errp)
>  {
>  GString *unavailable = NULL;
> @@ -1181,7 +1194,7 @@ static bool qmp_caps_accept(Monitor *mon, 
> QMPCapabilityList *list,
>  memset(capab, 0, sizeof(capab));
>  
>  for (; list; list = list->next) {
> -if 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/6] configure: Try to fix --static linking

[Peter Maydell]

On Fri, 14 Jun 2019 at 08:27, Philippe Mathieu-Daudé  wrote:
> Apparently QEMU static linking is slowly bitroting. Obviously it
> depends the libraries an user has installed, anyway it seems there
> are not much testing done.

The main reason for supporting static linking is so we can build
the user-mode emulators. Almost always the problems with
static linking the softmmu binaries and the tools are
issues with the distro's packaging of the static libraries
(pkg-config files which specify things that don't work for
static is a common one).

So we could put in a lot of checking of "is what pkg-config
tells us broken". Or we could just say "we don't support static
linking for anything except the usermode binaries". We
should probably phase in deprecation of that because it's
possible somebody's using it seriously, but it seems like
a fairly weird thing to do to me.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH v3 04/15] monitor: Create MonitorHMP with readline state

[Markus Armbruster]

Kevin Wolf  writes:

> The ReadLineState in Monitor is only used for HMP monitors. Create
> MonitorHMP and move it there.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Dr. David Alan Gilbert 
> ---
>  include/monitor/monitor.h |   5 +-
>  hmp.c |   4 +-
>  monitor.c | 129 +-
>  3 files changed, 79 insertions(+), 59 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 06cfcd8f36..efdea83bb3 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -6,6 +6,7 @@
>  #include "qemu/readline.h"
>  
>  extern __thread Monitor *cur_mon;
> +typedef struct MonitorHMP MonitorHMP;
>  
>  /* flags for monitor_init */
>  /* 0x01 unused */
> @@ -34,8 +35,8 @@ void monitor_flush(Monitor *mon);
>  int monitor_set_cpu(int cpu_index);
>  int monitor_get_cpu_index(void);
>  
> -void monitor_read_command(Monitor *mon, int show_prompt);
> -int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> +void monitor_read_command(MonitorHMP *mon, int show_prompt);
> +int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
>void *opaque);
>  
>  AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> diff --git a/hmp.c b/hmp.c
> index be5e345c6f..99414cd39c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1943,6 +1943,8 @@ static void hmp_change_read_arg(void *opaque, const 
> char *password,
>  
>  void hmp_change(Monitor *mon, const QDict *qdict)
>  {
> +/* FIXME Make MonitorHMP public and use container_of */

I think this FIXME should be resolved in PATCH 09.  Recommend to mention
in the commit message.  Can give it a try in my tree.

> +MonitorHMP *hmp_mon = (MonitorHMP *) mon;

Drop the space before mon.  Can touch up in my tree.

>  const char *device = qdict_get_str(qdict, "device");
>  const char *target = qdict_get_str(qdict, "target");
>  const char *arg = qdict_get_try_str(qdict, "arg");
> @@ -1960,7 +1962,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>  if (strcmp(target, "passwd") == 0 ||
>  strcmp(target, "password") == 0) {
>  if (!arg) {
> -monitor_read_password(mon, hmp_change_read_arg, NULL);
> +monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
>  return;
>  }
>  }
> diff --git a/monitor.c b/monitor.c
> index 855a147723..572449f6db 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -192,14 +192,6 @@ struct Monitor {
>  bool skip_flush;
>  bool use_io_thread;
>  
> -/*
> - * State used only in the thread "owning" the monitor.
> - * If @use_io_thread, this is @mon_iothread.
> - * Else, it's the main thread.
> - * These members can be safely accessed without locks.
> - */
> -ReadLineState *rs;
> -
>  gchar *mon_cpu_path;
>  mon_cmd_t *cmd_table;
>  QTAILQ_ENTRY(Monitor) entry;
> @@ -220,6 +212,18 @@ struct Monitor {
>  int mux_out;
>  };
>  
> +struct MonitorHMP {
> +Monitor common;
> +/*
> + * State used only in the thread "owning" the monitor.
> + * If @use_io_thread, this is @mon_iothread. (This does not actually 
> happen
> + * in the current state of the code.)
> + * Else, it's the main thread.
> + * These members can be safely accessed without locks.
> + */
> +ReadLineState *rs;
> +};
> +
>  typedef struct {
>  Monitor common;
>  JSONMessageParser parser;
> @@ -326,7 +330,7 @@ bool monitor_cur_is_qmp(void)
>  return cur_mon && monitor_is_qmp(cur_mon);
>  }
>  
> -void monitor_read_command(Monitor *mon, int show_prompt)
> +void monitor_read_command(MonitorHMP *mon, int show_prompt)
>  {
>  if (!mon->rs)
>  return;
> @@ -336,7 +340,7 @@ void monitor_read_command(Monitor *mon, int show_prompt)
>  readline_show_prompt(mon->rs);
>  }
>  
> -int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> +int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
>void *opaque)
>  {
>  if (mon->rs) {
> @@ -344,7 +348,8 @@ int monitor_read_password(Monitor *mon, ReadLineFunc 
> *readline_func,
>  /* prompt is printed on return from the command handler */
>  return 0;
>  } else {
> -monitor_printf(mon, "terminal does not support password 
> prompting\n");
> +monitor_printf(>common,
> +   "terminal does not support password prompting\n");
>  return -ENOTTY;
>  }
>  }
> @@ -705,7 +710,7 @@ static void monitor_qapi_event_init(void)
>  qapi_event_throttle_equal);
>  }
>  
> -static void handle_hmp_command(Monitor *mon, const char *cmdline);
> +static void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
>  
>  static void monitor_iothread_init(void);
>  
> @@ -715,7 +720,6 @@ static void 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 02/15] monitor: Split monitor_init in HMP and QMP function

[Markus Armbruster]

Kevin Wolf  writes:

> Instead of mixing HMP and QMP monitors in the same function, separate
> the monitor creation function for both.
>
> While in theory, one could pass both MONITOR_USE_CONTROL and
> MONITOR_USE_READLINE before this patch and both flags would do
> something, readline support is tightly coupled with HMP: QMP never feeds
> its input to readline, and the tab completion function treats the input
> as an HMP command. Therefore, this configuration is useless.
>
> After this patch, the QMP path asserts that MONITOR_USE_READLINE is not
> set. The HMP path can be used with or without MONITOR_USE_READLINE, like
> before.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Dr. David Alan Gilbert 
> Reviewed-by: Markus Armbruster 
> ---
>  monitor.c | 89 ---
>  1 file changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 9846a5623b..a70c1283b1 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -704,7 +704,7 @@ static void handle_hmp_command(Monitor *mon, const char 
> *cmdline);
>  
>  static void monitor_iothread_init(void);
>  
> -static void monitor_data_init(Monitor *mon, bool skip_flush,
> +static void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
>bool use_io_thread)
>  {
>  if (use_io_thread && !mon_iothread) {
   monitor_iothread_init();
   }
   memset(mon, 0, sizeof(Monitor));

I'd like to replace this memset() by ...

   qemu_mutex_init(>mon_lock);
   qemu_mutex_init(>qmp.qmp_queue_lock);
   mon->outbuf = qstring_new();
   /* Use *mon_cmds by default. */
   mon->cmd_table = mon_cmds;
> @@ -719,6 +719,7 @@ static void monitor_data_init(Monitor *mon, bool 
> skip_flush,
>  mon->skip_flush = skip_flush;
>  mon->use_io_thread = use_io_thread;
>  mon->qmp.qmp_requests = g_queue_new();
> +mon->flags = flags;
>  }
>  
>  static void monitor_data_destroy(Monitor *mon)
> @@ -742,7 +743,7 @@ char *qmp_human_monitor_command(const char *command_line, 
> bool has_cpu_index,
>  char *output = NULL;
>  Monitor *old_mon, hmp;

... hmp = {} here (moved from PATCH 04), and ...

>  
> -monitor_data_init(, true, false);
> +monitor_data_init(, 0, true, false);
>  
>  old_mon = cur_mon;
>  cur_mon = 
> @@ -4605,19 +4606,51 @@ static void monitor_qmp_setup_handlers_bh(void 
> *opaque)
>  monitor_list_append(mon);
>  }
>  
> -void monitor_init(Chardev *chr, int flags)
> +static void monitor_init_qmp(Chardev *chr, int flags)
>  {
>  Monitor *mon = g_malloc(sizeof(*mon));

... g_malloc0() here (moved from PATCH 03), and ...

> -bool use_readline = flags & MONITOR_USE_READLINE;
> +
> +/* Only HMP supports readline */
> +assert(!(flags & MONITOR_USE_READLINE));
>  
>  /* Note: we run QMP monitor in I/O thread when @chr supports that */
> -monitor_data_init(mon, false,
> -  (flags & MONITOR_USE_CONTROL)
> -  && qemu_chr_has_feature(chr,
> -  QEMU_CHAR_FEATURE_GCONTEXT));
> +monitor_data_init(mon, flags, false,
> +  qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
>  
>  qemu_chr_fe_init(>chr, chr, _abort);
> -mon->flags = flags;
> +qemu_chr_fe_set_echo(>chr, true);
> +
> +json_message_parser_init(>qmp.parser, handle_qmp_command, mon, 
> NULL);
> +if (mon->use_io_thread) {
> +/*
> + * Make sure the old iowatch is gone.  It's possible when
> + * e.g. the chardev is in client mode, with wait=on.
> + */
> +remove_fd_in_watch(chr);
> +/*
> + * We can't call qemu_chr_fe_set_handlers() directly here
> + * since chardev might be running in the monitor I/O
> + * thread.  Schedule a bottom half.
> + */
> +aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +monitor_qmp_setup_handlers_bh, mon);
> +/* The bottom half will add @mon to @mon_list */
> +} else {
> +qemu_chr_fe_set_handlers(>chr, monitor_can_read,
> + monitor_qmp_read, monitor_qmp_event,
> + NULL, mon, NULL, true);
> +monitor_list_append(mon);
> +}
> +}
> +
> +static void monitor_init_hmp(Chardev *chr, int flags)
> +{
> +Monitor *mon = g_malloc(sizeof(*mon));

... and g_malloc0() here (moved from PATCH 04).

This way, the responsibility for zeroing moves just once, right when its
new owners get created.  Saves us explaining in PATCH 03+04 why we make
these changes.  They were confusing enough for me to ask for an
explanation in my review of v2.

Happy do to it in my tree.

I'd be tempted to assert(!(flags & MONITOR_USE_CONTROL)) here, and the
opposite in monitor_init_qmp(), if your PATCH 14 didn't get rid of
@flags.  Okay as it is.

> +bool use_readline = flags & 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 14/15] monitor: Replace monitor_init() with monitor_init_{hmp, qmp}()

[Markus Armbruster]

Kevin Wolf  writes:

> Most callers know which monitor type they want to have. Instead of
> calling monitor_init() with flags that can describe both types of
> monitors, make monitor_init_{hmp,qmp}() public interfaces that take
> specific bools instead of flags and call these functions directly.
>
> Signed-off-by: Kevin Wolf 

Neater now.  Thanks!

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 13/15] monitor: Split Monitor.flags into separate bools

[Markus Armbruster]

Kevin Wolf  writes:

> Monitor.flags contains three different flags: One to distinguish HMP
> from QMP; one specific to HMP (MONITOR_USE_READLINE) that is ignored
> with QMP; and another one specific to QMP (MONITOR_USE_PRETTY) that is
> ignored with HMP.
>
> Split the flags field into three bools and move them to the right
> subclass. Flags are still in use for the monitor_init() interface.
>
> Signed-off-by: Kevin Wolf 
> ---
>  monitor/monitor-internal.h |  8 +---
>  monitor/hmp.c  |  6 +++---
>  monitor/misc.c |  2 +-
>  monitor/monitor.c  | 14 +-
>  monitor/qmp.c  |  7 ---
>  5 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 30679d522e..260725e51b 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -90,8 +90,8 @@ typedef struct HMPCommand {
>  struct Monitor {
>  CharBackend chr;
>  int reset_seen;
> -int flags;
>  int suspend_cnt;/* Needs to be accessed atomically */
> +bool is_qmp;
>  bool skip_flush;
>  bool use_io_thread;
>  
> @@ -116,6 +116,7 @@ struct Monitor {
>  
>  struct MonitorHMP {
>  Monitor common;
> +bool use_readline;
>  /*
>   * State used only in the thread "owning" the monitor.
>   * If @use_io_thread, this is @mon_iothread. (This does not actually 
> happen
> @@ -129,6 +130,7 @@ struct MonitorHMP {
>  typedef struct {
>  Monitor common;
>  JSONMessageParser parser;
> +bool pretty;
>  /*
>   * When a client connects, we're in capabilities negotiation mode.
>   * @commands is _cap_negotiation_commands then.  When command
> @@ -152,7 +154,7 @@ typedef struct {
>   */
>  static inline bool monitor_is_qmp(const Monitor *mon)
>  {
> -return (mon->flags & MONITOR_USE_CONTROL);
> +return mon->is_qmp;
>  }

The function was marginal before, and it's pointless now.  Let's kill it
in a follow-up patch.  No need to do it in this series.

>  
>  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
> @@ -169,7 +171,7 @@ void monitor_init_qmp(Chardev *chr, int flags);
>  void monitor_init_hmp(Chardev *chr, int flags);
>  
>  int monitor_puts(Monitor *mon, const char *str);
> -void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
> +void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
> bool use_io_thread);
>  void monitor_data_destroy(Monitor *mon);
>  int monitor_can_read(void *opaque);
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 3621b195ed..5ba8b6e8d5 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1396,12 +1396,12 @@ static void monitor_readline_flush(void *opaque)
>  void monitor_init_hmp(Chardev *chr, int flags)
>  {
>  MonitorHMP *mon = g_malloc0(sizeof(*mon));
> -bool use_readline = flags & MONITOR_USE_READLINE;
>  
> -monitor_data_init(>common, flags, false, false);
> +monitor_data_init(>common, false, false, false);
>  qemu_chr_fe_init(>common.chr, chr, _abort);
>  
> -if (use_readline) {
> +mon->use_readline = flags & MONITOR_USE_READLINE;
> +if (mon->use_readline) {
>  mon->rs = readline_init(monitor_readline_printf,
>  monitor_readline_flush,
>  mon,
> diff --git a/monitor/misc.c b/monitor/misc.c
> index dddbddb21f..10c056394e 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -134,7 +134,7 @@ char *qmp_human_monitor_command(const char *command_line, 
> bool has_cpu_index,
>  Monitor *old_mon;
>  MonitorHMP hmp = {};
>  
> -monitor_data_init(, 0, true, false);
> +monitor_data_init(, false, true, false);
>  
>  old_mon = cur_mon;
>  cur_mon = 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 6802b8e491..7325e4362b 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -80,14 +80,18 @@ bool monitor_cur_is_qmp(void)
>   * Note: not all HMP monitors use readline, e.g., gdbserver has a
>   * non-interactive HMP monitor, so readline is not used there.
>   */
> -static inline bool monitor_uses_readline(const Monitor *mon)
> +static inline bool monitor_uses_readline(const MonitorHMP *mon)
>  {
> -return mon->flags & MONITOR_USE_READLINE;
> +return mon->use_readline;
>  }

Another one to kill.

>  
>  static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
>  {
> -return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
> +if (monitor_is_qmp(mon)) {
> +return false;
> +}
> +
> +return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
>  }
>  

Not sure about this one.  We'll see.

>  static void monitor_flush_locked(Monitor *mon);
> @@ -523,17 +527,17 @@ static void monitor_iothread_init(void)
>  mon_iothread = iothread_create("mon_iothread", _abort);
>  }
>  
> -void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
> +void 

Re: [Qemu-block] [PATCH 0/6] configure: Try to fix --static linking

[Philippe Mathieu-Daudé]

On 6/14/19 9:24 AM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> Apparently QEMU static linking is slowly bitroting. Obviously it
> depends the libraries an user has installed, anyway it seems there
> are not much testing done.
> 
> This series fixes few issues, enough to build QEMU on a Ubuntu
> aarch64 host, but not yet on a x86_64 host:
> 
> LINKx86_64-softmmu/qemu-system-x86_64
>   /usr/bin/ld: cannot find -lgtk-3
>   /usr/bin/ld: cannot find -latk-bridge-2.0
>   /usr/bin/ld: cannot find -latspi
>   /usr/bin/ld: cannot find -lsystemd
>   /usr/bin/ld: cannot find -lgdk-3
>   /usr/bin/ld: cannot find -lwayland-egl
>   /usr/bin/ld: cannot find -lmirclient
>   /usr/bin/ld: cannot find -lmircore
>   /usr/bin/ld: cannot find -lmircookie
>   /usr/bin/ld: cannot find -lepoxy
>   /usr/bin/ld: cannot find -latk-1.0
>   /usr/bin/ld: cannot find -lgdk_pixbuf-2.0
>   /usr/bin/ld: cannot find -lselinux
>   /usr/bin/ld: cannot find -lgtk-3
>   /usr/bin/ld: cannot find -latk-bridge-2.0
>   /usr/bin/ld: cannot find -latspi
>   /usr/bin/ld: cannot find -lsystemd
>   /usr/bin/ld: cannot find -lgdk-3
>   /usr/bin/ld: cannot find -lwayland-egl
>   /usr/bin/ld: cannot find -lmirclient
>   /usr/bin/ld: cannot find -lmircore
>   /usr/bin/ld: cannot find -lmircookie
>   /usr/bin/ld: cannot find -lepoxy
>   /usr/bin/ld: cannot find -latk-1.0
>   /usr/bin/ld: cannot find -lgdk_pixbuf-2.0
>   /usr/bin/ld: cannot find -lselinux
>   /usr/bin/ld: attempted static link of dynamic object 
> `/usr/lib/x86_64-linux-gnu/libz.so'
>   collect2: error: ld returned 1 exit status

This one is funny, when installing libvte on Ubuntu 18.04:

LINKx86_64-softmmu/qemu-system-x86_64
  c++: error: /usr/lib/x86_64-linux-gnu/libunistring.so: No such file or
directory
  c++: error: /usr/lib/x86_64-linux-gnu/libunistring.so: No such file or
directory
  c++: error: /usr/lib/x86_64-linux-gnu/libunistring.so: No such file or
directory
  c++: error: /usr/lib/x86_64-linux-gnu/libunistring.so: No such file or
directory

$ pkg-config --libs --static vte-2.91
-lvte-2.91 -lgtk-3 -latk-bridge-2.0 -latspi -ldbus-1 -lpthread -lsystemd
-lgdk-3 -lXinerama -lXi -lXrandr -lXcursor -lXcomposite -lXdamage
-lXfixes -lxkbcommon -lwayland-cursor -lwayland-egl -lwayland-client
-lepoxy -ldl -lpangocairo-1.0 -lpangoft2-1.0 -lharfbuzz -lm -lgraphite2
-lpango-1.0 -lm -latk-1.0 -lcairo-gobject -lcairo -lz -lpixman-1
-lfontconfig -lexpat -lfreetype -lexpat -lfreetype -lpng16 -lm -lz -lm
-lxcb-shm -lxcb-render -lXrender -lXext -lX11 -lpthread -lxcb -lXau
-lXdmcp -lgdk_pixbuf-2.0 -lm -lpng16 -lm -lz -lm -lgio-2.0 -lz -lresolv
-lselinux -lmount -lgmodule-2.0 -pthread -ldl -lgobject-2.0 -lffi
-lglib-2.0 -pthread -lpcre -pthread -lgnutls -lgmp
/usr/lib/x86_64-linux-gnu/libunistring.so -lidn2 -lhogweed -lgmp
-lnettle -ltasn1 -lp11-kit -lz

$ ls -ld /usr/lib/x86_64-linux-gnu/libunistring.so
ls: cannot access '/usr/lib/x86_64-linux-gnu/libunistring.so': No such
file or directory

$ ls -ld /usr/lib/x86_64-linux-gnu/libunistring.so*
lrwxrwxrwx. 1 root root  21 Mar  3  2018
/usr/lib/x86_64-linux-gnu/libunistring.so.2 -> libunistring.so.2.1.0
-rw-r--r--. 1 root root 1562664 Mar  3  2018
/usr/lib/x86_64-linux-gnu/libunistring.so.2.1.0

The fix is probably "sudo ln -s libunistring.so.2
/usr/lib/x86_64-linux-gnu/libunistring.so".

Re: [Qemu-block] [Qemu-devel] [PATCH v3 09/15] monitor: Create monitor-internal.h with common definitions

[Kevin Wolf]

Am 14.06.2019 um 08:37 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Before we can split monitor/misc.c, we need to create a header file that
> > contains the common definitions that will be used by multiple source
> > files.
> >
> > For a start, add the type definitions for Monitor, MonitorHMP and
> > MonitorQMP and their dependencies. We'll add functions as needed when
> > splitting monitor/misc.c.
> >
> > Signed-off-by: Kevin Wolf 
> > Reviewed-by: Dr. David Alan Gilbert 
> > ---
> >  monitor/monitor-internal.h | 148 +
> >  monitor/misc.c | 110 +--
> >  MAINTAINERS|   2 +
> >  3 files changed, 151 insertions(+), 109 deletions(-)
> >  create mode 100644 monitor/monitor-internal.h
> >
> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> > new file mode 100644
> > index 00..17a632b0ad
> > --- /dev/null
> > +++ b/monitor/monitor-internal.h
> > @@ -0,0 +1,148 @@
> > +/*
> > + * QEMU monitor
> > + *
> > + * Copyright (c) 2003-2004 Fabrice Bellard
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> > + * of this software and associated documentation files (the "Software"), 
> > to deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
> > IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#ifndef MONITOR_INT_H
> > +#define MONITOR_INT_H
> 
> Rename to MONITOR_INTERNAL_H, so it again matches the file name.  Can
> touch up in my tree.

Oops, yes, please do.

> > +
> > +#include "monitor/monitor.h"
> > +#include "qapi/qmp/qdict.h"
> 
> These too are superfluous.  I'm willing to tolerate monitor.h anyway,
> since anything including monitor-internal.h is almost certainly going to
> need monitor.h, too.

I tried to drop them because you suggested so, but it results in compile
errors. On closer look, I think qdict.h can go because the typedef will
be present through qemu/osdep.h, which must be included before this one,
but MonitorHMP is only defined by monitor/monitor.h.

> > +#include "qapi/qmp/json-parser.h"
> > +#include "qapi/qmp/dispatch.h"
> > +#include "qapi/qapi-types-misc.h"
> > +
> > +#include "qemu/readline.h"
> > +#include "chardev/char-fe.h"
> > +#include "sysemu/iothread.h"
> 
> Another superfluous one.

IOThread is only defined by system/iothread.h, so I don't think you can
remove this one either.

> Happy to drop these two #include in my tree.

As long as the result still builds, feel free to drop includes from the
header (and probably add them to source files instead where they will be
missing then).

Kevin