Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data

[John Snow]

On 11/02/2016 03:49 PM, Hervé Poussineau wrote:

Le 01/11/2016 à 04:16, John Snow a écrit :

v2:
 - Actually applied the changes this time ...
 - And added a test to the AHCI suite...
 - ...Which revealed a few small issues in the suite.

The AHCI test should be sufficient in terms of general proof
for ATAPI regardless of the HBA used.



All patches:
Tested-by: Hervé Poussineau 



Great, thank you. Sorry it took so long for me to act on your original 
patch.






For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ide-fix-read-cd
https://github.com/jnsnow/qemu/tree/ide-fix-read-cd

This version is tagged ide-fix-read-cd-v2:
https://github.com/jnsnow/qemu/releases/tag/ide-fix-read-cd-v2

John Snow (3):
  atapi: classify read_cd as conditionally returning data
  ahci-test: Create smaller test ISO images
  ahci-test: test atapi read_cd with bcl,nb_sectors = 0

 hw/ide/atapi.c  | 51
---
 tests/ahci-test.c   | 39 +++
 tests/libqos/ahci.c | 27 ---
 tests/libqos/ahci.h | 17 ++---
 4 files changed, 101 insertions(+), 33 deletions(-)

Re: [Qemu-block] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data

[Hervé Poussineau]

Le 01/11/2016 à 04:16, John Snow a écrit :

v2:
 - Actually applied the changes this time ...
 - And added a test to the AHCI suite...
 - ...Which revealed a few small issues in the suite.

The AHCI test should be sufficient in terms of general proof
for ATAPI regardless of the HBA used.



All patches:
Tested-by: Hervé Poussineau 




For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ide-fix-read-cd
https://github.com/jnsnow/qemu/tree/ide-fix-read-cd

This version is tagged ide-fix-read-cd-v2:
https://github.com/jnsnow/qemu/releases/tag/ide-fix-read-cd-v2

John Snow (3):
  atapi: classify read_cd as conditionally returning data
  ahci-test: Create smaller test ISO images
  ahci-test: test atapi read_cd with bcl,nb_sectors = 0

 hw/ide/atapi.c  | 51 ---
 tests/ahci-test.c   | 39 +++
 tests/libqos/ahci.c | 27 ---
 tests/libqos/ahci.h | 17 ++---
 4 files changed, 101 insertions(+), 33 deletions(-)

Re: [Qemu-block] [PATCH for-2.8? 2/3] qapi: Drop curl's TFTP protocol

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:38PM +0100, Max Reitz wrote:
> A follow-up patch will remove the curl block driver's TFTP support, so
> remove the protocol from the QAPI schema.
> 
> Signed-off-by: Max Reitz 
> ---
>  docs/qmp-commands.txt | 2 +-
>  qapi/block-core.json  | 7 +++
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> index 6afa872..abf210a 100644
> --- a/docs/qmp-commands.txt
> +++ b/docs/qmp-commands.txt
> @@ -1803,7 +1803,7 @@ Each json-object contain the following:
>  "file", "file", "ftp", "ftps", "host_cdrom",
>  "host_device", "http", "https",
>  "nbd", "parallels", "qcow", "qcow2", "raw",
> -"tftp", "vdi", "vmdk", "vpc", "vvfat"
> +"vdi", "vmdk", "vpc", "vvfat"
>   - "backing_file": backing file name (json-string, optional)
>   - "backing_file_depth": number of files in the backing file chain 
> (json-int)
>   - "encrypted": true if encrypted, false otherwise (json-bool)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index bcd3b9e..c29bef7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -243,12 +243,12 @@
>  #   0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
>  #   'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
>  #   'http', 'https', 'luks', 'nbd', 'parallels', 'qcow',
> -#   'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat'
> +#   'qcow2', 'raw', 'vdi', 'vmdk', 'vpc', 'vvfat'
>  #   2.2: 'archipelago' added, 'cow' dropped
>  #   2.3: 'host_floppy' deprecated
>  #   2.5: 'host_floppy' dropped
>  #   2.6: 'luks' added
> -#   2.8: 'replication' added
> +#   2.8: 'replication' added, 'tftp' dropped
>  #
>  # @backing_file: #optional the name of the backing file (for copy-on-write)
>  #
> @@ -1723,7 +1723,7 @@
>  'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>  'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
>  'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> -'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
> +'replication', 'ssh', 'vdi', 'vhdx', 'vmdk', 'vpc',
>  'vvfat' ] }
>  
>  ##
> @@ -2410,7 +2410,6 @@
>'replication':'BlockdevOptionsReplication',
>  # TODO sheepdog: Wait for structured options
>'ssh':'BlockdevOptionsSsh',
> -  'tftp':   'BlockdevOptionsCurl',
>'vdi':'BlockdevOptionsGenericFormat',
>'vhdx':   'BlockdevOptionsGenericFormat',
>'vmdk':   'BlockdevOptionsGenericCOWFormat',
> -- 
> 2.10.2
>

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH for-2.8? 1/3] qemu-options: Drop mentions of curl's TFTP support

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:37PM +0100, Max Reitz wrote:
> A follow-up patch will remove the curl block driver's TFTP support;
> therefore, we should no longer mention it anywhere in our documentation.
> 
> Signed-off-by: Max Reitz 
> ---
>  qemu-options.hx | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 95332cc..7212779 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2606,8 +2606,8 @@ qemu-system-x86_64 --drive 
> file=gluster://192.0.2.1/testvol/a.img
>  
>  See also @url{http://www.gluster.org}.
>  
> -@item HTTP/HTTPS/FTP/FTPS/TFTP
> -QEMU supports read-only access to files accessed over http(s), ftp(s) and 
> tftp.
> +@item HTTP/HTTPS/FTP/FTPS
> +QEMU supports read-only access to files accessed over http(s) and ftp(s).
>  
>  Syntax using a single filename:
>  @example
> @@ -2617,7 +2617,7 @@ Syntax using a single filename:
>  where:
>  @table @option
>  @item protocol
> -'http', 'https', 'ftp', 'ftps', or 'tftp'.
> +'http', 'https', 'ftp', or 'ftps'.
>  
>  @item username
>  Optional username for authentication to the remote server.
> -- 
> 2.10.2
>

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Jeff Cody]

On Wed, Nov 02, 2016 at 06:55:36PM +0100, Max Reitz wrote:
> See patch 3 for the reason why we have actually never supported TFTP at
> all (except for very small files (i.e. below 256 kB or so)).
> 
> I would consider this series a bug fix because, well, it doesn't really
> change any functionality, and the bug is "We don't support TFTP but we
> pretend we do".
> 

I tend to agree.  I'm willing to pull this in through my branch for 2.8,
unless there arises some outcry with good reason to keep tftp.

> 
> Alternatives to this approach:
> 
> - Deprecate TFTP first. Wait one version, then drop it.
> 
>   We could do this, but I personally don't think it's necessary. We have
>   done this for host_floppy, but in contrast to host_floppy, TFTP really
>   has never worked. Thus, I conclude that nobody is actually using it or
>   has ever used it for real work.
> 
>   Still, if you think otherwise, we can still do this, of course.
> 
> 
> - Don't remove TFTP altogether, but just emit a run-time error like we
>   do for HTTP servers that do not support range-based requests.
> 
>   Seems dirty and not like the real solution to me. Also, we have
>   removed other block drivers in the past, so I don't think we should
>   keep TFTP.
> 

Since it is broken by nature, I like your original approach of just removing
it.

> 
> Max Reitz (3):
>   qemu-options: Drop mentions of curl's TFTP support
>   qapi: Drop curl's TFTP protocol
>   block/curl: Drop TFTP "support"
> 
>  block/curl.c  | 20 +---
>  docs/qmp-commands.txt |  2 +-
>  qapi/block-core.json  |  7 +++
>  qemu-options.hx   |  6 +++---
>  4 files changed, 8 insertions(+), 27 deletions(-)

A > 3:1 delete to insert ratio, that is an ideal patch series ;-)

Re: [Qemu-block] [PULL v2 00/14] Block patches for 2.8

[Peter Maydell]

On 2 November 2016 at 17:03, Stefan Hajnoczi  wrote:
> On Tue, Nov 01, 2016 at 08:50:57AM -0400, Jeff Cody wrote:
>> The following changes since commit bf99fd3983d7185178a0f65ce29bb94b1aecaed1:
>>
>>   Merge remote-tracking branch 'remotes/rth/tags/pull-sparc-20161031-2' into 
>> staging (2016-11-01 11:21:02 +)
>>
>> are available in the git repository at:
>>
>>   g...@github.com:codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> This pull request does not have a publicly-accessible URI.
>
> Please adjust your .gitconfig:
>
> [remote "public"]
> url = git://github.com/codyprime/qemu.git
> pushurl = g...@github.com:codyprime/qemu.git

I think the apply-pullreq script can cope with that,
once you've added the remote to your git repo --
there's slack in the "figure out the remote given
the URL" logic to cope with github having a bunch
of different ways to represent the same repo and
maintainers tending to flip between them.

thanks
-- PMM

[Qemu-block] [PATCH for-2.8? 2/3] qapi: Drop curl's TFTP protocol

[Max Reitz]

A follow-up patch will remove the curl block driver's TFTP support, so
remove the protocol from the QAPI schema.

Signed-off-by: Max Reitz 
---
 docs/qmp-commands.txt | 2 +-
 qapi/block-core.json  | 7 +++
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index 6afa872..abf210a 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -1803,7 +1803,7 @@ Each json-object contain the following:
 "file", "file", "ftp", "ftps", "host_cdrom",
 "host_device", "http", "https",
 "nbd", "parallels", "qcow", "qcow2", "raw",
-"tftp", "vdi", "vmdk", "vpc", "vvfat"
+"vdi", "vmdk", "vpc", "vvfat"
  - "backing_file": backing file name (json-string, optional)
  - "backing_file_depth": number of files in the backing file chain 
(json-int)
  - "encrypted": true if encrypted, false otherwise (json-bool)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bcd3b9e..c29bef7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -243,12 +243,12 @@
 #   0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
 #   'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
 #   'http', 'https', 'luks', 'nbd', 'parallels', 'qcow',
-#   'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat'
+#   'qcow2', 'raw', 'vdi', 'vmdk', 'vpc', 'vvfat'
 #   2.2: 'archipelago' added, 'cow' dropped
 #   2.3: 'host_floppy' deprecated
 #   2.5: 'host_floppy' dropped
 #   2.6: 'luks' added
-#   2.8: 'replication' added
+#   2.8: 'replication' added, 'tftp' dropped
 #
 # @backing_file: #optional the name of the backing file (for copy-on-write)
 #
@@ -1723,7 +1723,7 @@
 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
 'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
+'replication', 'ssh', 'vdi', 'vhdx', 'vmdk', 'vpc',
 'vvfat' ] }
 
 ##
@@ -2410,7 +2410,6 @@
   'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
   'ssh':'BlockdevOptionsSsh',
-  'tftp':   'BlockdevOptionsCurl',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
   'vmdk':   'BlockdevOptionsGenericCOWFormat',
-- 
2.10.2

[Qemu-block] [PATCH for-2.8? 3/3] block/curl: Drop TFTP "support"

[Max Reitz]

Because TFTP does not support byte ranges, it was never usable with our
curl block driver. Since apparently nobody has ever complained loudly
enough for someone to take care of the issue until now, it seems
reasonable to assume that nobody has ever actually used it.

Therefore, it should be safe to just drop it from curl's protocol list.

Signed-off-by: Max Reitz 
---
 block/curl.c | 20 +---
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index e5eaa7b..ba8adae 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -68,8 +68,7 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 #endif
 
 #define PROTOCOLS (CURLPROTO_HTTP | CURLPROTO_HTTPS | \
-   CURLPROTO_FTP | CURLPROTO_FTPS | \
-   CURLPROTO_TFTP)
+   CURLPROTO_FTP | CURLPROTO_FTPS)
 
 #define CURL_NUM_STATES 8
 #define CURL_NUM_ACB8
@@ -886,29 +885,12 @@ static BlockDriver bdrv_ftps = {
 .bdrv_attach_aio_context= curl_attach_aio_context,
 };
 
-static BlockDriver bdrv_tftp = {
-.format_name= "tftp",
-.protocol_name  = "tftp",
-
-.instance_size  = sizeof(BDRVCURLState),
-.bdrv_parse_filename= curl_parse_filename,
-.bdrv_file_open = curl_open,
-.bdrv_close = curl_close,
-.bdrv_getlength = curl_getlength,
-
-.bdrv_aio_readv = curl_aio_readv,
-
-.bdrv_detach_aio_context= curl_detach_aio_context,
-.bdrv_attach_aio_context= curl_attach_aio_context,
-};
-
 static void curl_block_init(void)
 {
 bdrv_register(_http);
 bdrv_register(_https);
 bdrv_register(_ftp);
 bdrv_register(_ftps);
-bdrv_register(_tftp);
 }
 
 block_init(curl_block_init);
-- 
2.10.2

[Qemu-block] [PATCH for-2.8? 1/3] qemu-options: Drop mentions of curl's TFTP support

[Max Reitz]

A follow-up patch will remove the curl block driver's TFTP support;
therefore, we should no longer mention it anywhere in our documentation.

Signed-off-by: Max Reitz 
---
 qemu-options.hx | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 95332cc..7212779 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2606,8 +2606,8 @@ qemu-system-x86_64 --drive 
file=gluster://192.0.2.1/testvol/a.img
 
 See also @url{http://www.gluster.org}.
 
-@item HTTP/HTTPS/FTP/FTPS/TFTP
-QEMU supports read-only access to files accessed over http(s), ftp(s) and tftp.
+@item HTTP/HTTPS/FTP/FTPS
+QEMU supports read-only access to files accessed over http(s) and ftp(s).
 
 Syntax using a single filename:
 @example
@@ -2617,7 +2617,7 @@ Syntax using a single filename:
 where:
 @table @option
 @item protocol
-'http', 'https', 'ftp', 'ftps', or 'tftp'.
+'http', 'https', 'ftp', or 'ftps'.
 
 @item username
 Optional username for authentication to the remote server.
-- 
2.10.2

[Qemu-block] [PATCH v3 6/6] iotests: add transactional failure race test

[John Snow]

Add a regression test for the case found by Vladimir.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/124 | 53 ++
 tests/qemu-iotests/124.out |  4 ++--
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index f06938e..d0d2c2b 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -395,19 +395,7 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 self.check_backups()
 
 
-def test_transaction_failure(self):
-'''Test: Verify backups made from a transaction that partially fails.
-
-Add a second drive with its own unique pattern, and add a bitmap to 
each
-drive. Use blkdebug to interfere with the backup on just one drive and
-attempt to create a coherent incremental backup across both drives.
-
-verify a failure in one but not both, then delete the failed stubs and
-re-run the same transaction.
-
-verify that both incrementals are created successfully.
-'''
-
+def do_transaction_failure_test(self, race=False):
 # Create a second drive, with pattern:
 drive1 = self.add_node('drive1')
 self.img_create(drive1['file'], drive1['fmt'])
@@ -451,9 +439,10 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 self.assertFalse(self.vm.get_qmp_events(wait=False))
 
 # Emulate some writes
-self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
-  ('0xfe', '16M', '256k'),
-  ('0x64', '32736k', '64k')))
+if not race:
+self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+  ('0xfe', '16M', '256k'),
+  ('0x64', '32736k', '64k')))
 self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
   ('0xef', '16M', '256k'),
   ('0x46', '32736k', '64k')))
@@ -463,7 +452,8 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 target1 = self.prepare_backup(dr1bm0)
 
 # Ask for a new incremental backup per-each drive,
-# expecting drive1's backup to fail:
+# expecting drive1's backup to fail. In the 'race' test,
+# we expect drive1 to attempt to cancel the empty drive0 job.
 transaction = [
 transaction_drive_backup(drive0['id'], target0, sync='incremental',
  format=drive0['fmt'], mode='existing',
@@ -488,9 +478,15 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 self.assert_no_active_block_jobs()
 
 # Delete drive0's successful target and eliminate our record of the
-# unsuccessful drive1 target. Then re-run the same transaction.
+# unsuccessful drive1 target.
 dr0bm0.del_target()
 dr1bm0.del_target()
+if race:
+# Don't re-run the transaction, we only wanted to test the race.
+self.vm.shutdown()
+return
+
+# Re-run the same transaction:
 target0 = self.prepare_backup(dr0bm0)
 target1 = self.prepare_backup(dr1bm0)
 
@@ -511,6 +507,27 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 self.vm.shutdown()
 self.check_backups()
 
+def test_transaction_failure(self):
+'''Test: Verify backups made from a transaction that partially fails.
+
+Add a second drive with its own unique pattern, and add a bitmap to 
each
+drive. Use blkdebug to interfere with the backup on just one drive and
+attempt to create a coherent incremental backup across both drives.
+
+verify a failure in one but not both, then delete the failed stubs and
+re-run the same transaction.
+
+verify that both incrementals are created successfully.
+'''
+self.do_transaction_failure_test()
+
+def test_transaction_failure_race(self):
+'''Test: Verify that transactions with jobs that have no data to
+transfer do not cause race conditions in the cancellation of the entire
+transaction job group.
+'''
+self.do_transaction_failure_test(race=True)
+
 
 def test_sync_dirty_bitmap_missing(self):
 self.assert_no_active_block_jobs()
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 36376be..e56cae0 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-..
+...
 --
-Ran 10 tests
+Ran 11 tests
 
 OK
-- 
2.7.4

[Qemu-block] [PATCH for-2.8? 0/3] block/curl: Drop TFTP "support"

[Max Reitz]

See patch 3 for the reason why we have actually never supported TFTP at
all (except for very small files (i.e. below 256 kB or so)).

I would consider this series a bug fix because, well, it doesn't really
change any functionality, and the bug is "We don't support TFTP but we
pretend we do".


Alternatives to this approach:

- Deprecate TFTP first. Wait one version, then drop it.

  We could do this, but I personally don't think it's necessary. We have
  done this for host_floppy, but in contrast to host_floppy, TFTP really
  has never worked. Thus, I conclude that nobody is actually using it or
  has ever used it for real work.

  Still, if you think otherwise, we can still do this, of course.


- Don't remove TFTP altogether, but just emit a run-time error like we
  do for HTTP servers that do not support range-based requests.

  Seems dirty and not like the real solution to me. Also, we have
  removed other block drivers in the past, so I don't think we should
  keep TFTP.


Max Reitz (3):
  qemu-options: Drop mentions of curl's TFTP support
  qapi: Drop curl's TFTP protocol
  block/curl: Drop TFTP "support"

 block/curl.c  | 20 +---
 docs/qmp-commands.txt |  2 +-
 qapi/block-core.json  |  7 +++
 qemu-options.hx   |  6 +++---
 4 files changed, 8 insertions(+), 27 deletions(-)

-- 
2.10.2

[Qemu-block] [PATCH v3 2/6] blockjob: add .clean property

[John Snow]

Cleaning up after we have deferred to the main thread but before the
transaction has converged can be dangerous and result in deadlocks
if the job cleanup invokes any BH polling loops.

A job may attempt to begin cleaning up, but may induce another job to
enter its cleanup routine. The second job, part of our same transaction,
will block waiting for the first job to finish, so neither job may now
make progress.

To rectify this, allow jobs to register a cleanup operation that will
always run regardless of if the job was in a transaction or not, and
if the transaction job group completed successfully or not.

Move sensitive cleanup to this callback instead which is guaranteed to
be run only after the transaction has converged, which removes sensitive
timing constraints from said cleanup.

Furthermore, in future patches these cleanup operations will be performed
regardless of whether or not we actually started the job. Therefore,
cleanup callbacks should essentially confine themselves to undoing create
operations, e.g. setup actions taken in what is now backup_start.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 block/backup.c   | 15 ++-
 blockjob.c   |  3 +++
 include/block/blockjob_int.h |  8 
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 7b5d8a3..734a24c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -242,6 +242,14 @@ static void backup_abort(BlockJob *job)
 }
 }
 
+static void backup_clean(BlockJob *job)
+{
+BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+assert(s->target);
+blk_unref(s->target);
+s->target = NULL;
+}
+
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common);
@@ -321,6 +329,7 @@ static const BlockJobDriver backup_job_driver = {
 .set_speed  = backup_set_speed,
 .commit = backup_commit,
 .abort  = backup_abort,
+.clean  = backup_clean,
 .attached_aio_context   = backup_attached_aio_context,
 .drain  = backup_drain,
 };
@@ -343,12 +352,8 @@ typedef struct {
 
 static void backup_complete(BlockJob *job, void *opaque)
 {
-BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 BackupCompleteData *data = opaque;
 
-blk_unref(s->target);
-s->target = NULL;
-
 block_job_completed(job, data->ret);
 g_free(data);
 }
@@ -658,7 +663,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
 }
 if (job) {
-blk_unref(job->target);
+backup_clean(>common);
 block_job_unref(>common);
 }
 }
diff --git a/blockjob.c b/blockjob.c
index 4d0ef53..e3c458c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -241,6 +241,9 @@ static void block_job_completed_single(BlockJob *job)
 job->driver->abort(job);
 }
 }
+if (job->driver->clean) {
+job->driver->clean(job);
+}
 
 if (job->cb) {
 job->cb(job->opaque, job->ret);
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 40275e4..60d91a0 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -74,6 +74,14 @@ struct BlockJobDriver {
 void (*abort)(BlockJob *job);
 
 /**
+ * If the callback is not NULL, it will be invoked after a call to either
+ * .commit() or .abort(). Regardless of which callback is invoked after
+ * completion, .clean() will always be called, even if the job does not
+ * belong to a transaction group.
+ */
+void (*clean)(BlockJob *job);
+
+/**
  * If the callback is not NULL, it will be invoked when the job transitions
  * into the paused state.  Paused jobs must not perform any asynchronous
  * I/O or event loop activity.  This callback is used to quiesce jobs.
-- 
2.7.4

[Qemu-block] [PATCH v3 0/6] jobs: fix transactional race condition

[John Snow]

There are a few problems with transactional job completion right now.

First, if jobs complete so quickly they complete before remaining jobs
get a chance to join the transaction, the completion mode can leave well
known state and the QLIST can get corrupted and the transactional jobs
can complete in batches or phases instead of all together.

Second, if two or more jobs defer to the main loop at roughly the same
time, it's possible for one job's cleanup to directly invoke the other
job's cleanup from within the same thread, leading to a situation that
will deadlock the entire transaction.

Thanks to Vladimir for pointing out these modes of failure.

===
v3:
===

- Rebase to origin/master, requisite patches now upstream.

===
v2:
===

- Correct Vladimir's email (Sorry!)
- Add test as a variant of an existing test [Vladimir]



For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch job-fix-race-condition
https://github.com/jnsnow/qemu/tree/job-fix-race-condition

This version is tagged job-fix-race-condition-v3:
https://github.com/jnsnow/qemu/releases/tag/job-fix-race-condition-v3

John Snow (5):
  blockjob: add .clean property
  blockjob: add .start field
  blockjob: add block_job_start
  blockjob: refactor backup_start as backup_job_create
  iotests: add transactional failure race test

Vladimir Sementsov-Ogievskiy (1):
  blockjob: fix dead pointer in txn list

 block/backup.c   | 63 ++---
 block/commit.c   |  4 +--
 block/mirror.c   |  5 +--
 block/replication.c  | 12 ---
 block/stream.c   |  4 +--
 blockdev.c   | 83 
 blockjob.c   | 55 ++---
 include/block/block_int.h| 23 ++--
 include/block/blockjob.h |  9 +
 include/block/blockjob_int.h | 11 ++
 tests/qemu-iotests/124   | 53 ++--
 tests/qemu-iotests/124.out   |  4 +--
 tests/test-blockjob-txn.c| 12 +++
 13 files changed, 221 insertions(+), 117 deletions(-)

-- 
2.7.4

[Qemu-block] [PATCH v3 5/6] blockjob: refactor backup_start as backup_job_create

[John Snow]

Refactor backup_start as backup_job_create, which only creates the job,
but does not automatically start it. The old interface, 'backup_start',
is not kept in favor of limiting the number of nearly-identical interfaces
that would have to be edited to keep up with QAPI changes in the future.

Callers that wish to synchronously start the backup_block_job can
instead just call block_job_start immediately after calling
backup_job_create.

Transactions are updated to use the new interface, calling block_job_start
only during the .commit phase, which helps prevent race conditions where
jobs may finish before we even finish building the transaction. This may
happen, for instance, during empty block backup jobs.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 block/backup.c| 26 ---
 block/replication.c   | 12 ---
 blockdev.c| 83 ++-
 include/block/block_int.h | 23 ++---
 4 files changed, 87 insertions(+), 57 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index ae1b99a..ea38733 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -543,7 +543,7 @@ static const BlockJobDriver backup_job_driver = {
 .drain  = backup_drain,
 };
 
-void backup_start(const char *job_id, BlockDriverState *bs,
+BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, int64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
   bool compress,
@@ -563,52 +563,52 @@ void backup_start(const char *job_id, BlockDriverState 
*bs,
 
 if (bs == target) {
 error_setg(errp, "Source and target cannot be the same");
-return;
+return NULL;
 }
 
 if (!bdrv_is_inserted(bs)) {
 error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(bs));
-return;
+return NULL;
 }
 
 if (!bdrv_is_inserted(target)) {
 error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(target));
-return;
+return NULL;
 }
 
 if (compress && target->drv->bdrv_co_pwritev_compressed == NULL) {
 error_setg(errp, "Compression is not supported for this drive %s",
bdrv_get_device_name(target));
-return;
+return NULL;
 }
 
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
-return;
+return NULL;
 }
 
 if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
-return;
+return NULL;
 }
 
 if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 if (!sync_bitmap) {
 error_setg(errp, "must provide a valid bitmap name for "
  "\"incremental\" sync mode");
-return;
+return NULL;
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
 if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
-return;
+return NULL;
 }
 } else if (sync_bitmap) {
 error_setg(errp,
"a sync_bitmap was provided to backup_run, "
"but received an incompatible sync_mode (%s)",
MirrorSyncMode_lookup[sync_mode]);
-return;
+return NULL;
 }
 
 len = bdrv_getlength(bs);
@@ -655,8 +655,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 block_job_add_bdrv(>common, target);
 job->common.len = len;
 block_job_txn_add_job(txn, >common);
-block_job_start(>common);
-return;
+
+return >common;
 
  error:
 if (sync_bitmap) {
@@ -666,4 +666,6 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 backup_clean(>common);
 block_job_unref(>common);
 }
+
+return NULL;
 }
diff --git a/block/replication.c b/block/replication.c
index d5e2b0f..729dd12 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -421,6 +421,7 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 int64_t active_length, hidden_length, disk_length;
 AioContext *aio_context;
 Error *local_err = NULL;
+BlockJob *job;
 
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
@@ -508,17 +509,18 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 bdrv_op_block_all(top_bs, s->blocker);
 bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-backup_start(NULL, s->secondary_disk->bs, s->hidden_disk->bs, 0,
- MIRROR_SYNC_MODE_NONE, NULL, false,
- BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
- BLOCK_JOB_INTERNAL, backup_job_completed, bs,
- NULL, _err);
+job = 

[Qemu-block] [PATCH v3 4/6] blockjob: add block_job_start

[John Snow]

Instead of automatically starting jobs at creation time via backup_start
et al, we'd like to return a job object pointer that can be started
manually at later point in time.

For now, add the block_job_start mechanism and start the jobs
automatically as we have been doing, with conversions job-by-job coming
in later patches.

Of note: cancellation of unstarted jobs will perform all the normal
cleanup as if the job had started, particularly abort and clean. The
only difference is that we will not emit any events, because the job
never actually started.

Signed-off-by: John Snow 
---
 block/backup.c|  3 +--
 block/commit.c|  3 +--
 block/mirror.c|  3 +--
 block/stream.c|  3 +--
 blockjob.c| 51 ---
 include/block/blockjob.h  |  9 +
 tests/test-blockjob-txn.c | 12 +--
 7 files changed, 58 insertions(+), 26 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 4ed4494..ae1b99a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -654,9 +654,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 
 block_job_add_bdrv(>common, target);
 job->common.len = len;
-job->common.co = qemu_coroutine_create(job->common.driver->start, job);
 block_job_txn_add_job(txn, >common);
-qemu_coroutine_enter(job->common.co);
+block_job_start(>common);
 return;
 
  error:
diff --git a/block/commit.c b/block/commit.c
index 20d27e2..5b7c454 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -289,10 +289,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 
 trace_commit_start(bs, base, top, s, s->common.co);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
 
 
diff --git a/block/mirror.c b/block/mirror.c
index 659e09c..c078d45 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1009,9 +1009,8 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 }
 }
 
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_mirror_start(bs, s, s->common.co, opaque);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 92309ff..2de8d38 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -255,7 +255,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 s->bs_flags = orig_bs_flags;
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_stream_start(bs, base, s, s->common.co);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
diff --git a/blockjob.c b/blockjob.c
index e3c458c..16c5159 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -174,7 +174,8 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 job->blk   = blk;
 job->cb= cb;
 job->opaque= opaque;
-job->busy  = true;
+job->busy  = false;
+job->paused= true;
 job->refcnt= 1;
 bs->job = job;
 
@@ -202,6 +203,21 @@ bool block_job_is_internal(BlockJob *job)
 return (job->id == NULL);
 }
 
+static bool block_job_started(BlockJob *job)
+{
+return job->co;
+}
+
+void block_job_start(BlockJob *job)
+{
+assert(job && !block_job_started(job) && job->paused &&
+   !job->busy && job->driver->start);
+job->paused = false;
+job->busy = true;
+job->co = qemu_coroutine_create(job->driver->start, job);
+qemu_coroutine_enter(job->co);
+}
+
 void block_job_ref(BlockJob *job)
 {
 ++job->refcnt;
@@ -248,14 +264,18 @@ static void block_job_completed_single(BlockJob *job)
 if (job->cb) {
 job->cb(job->opaque, job->ret);
 }
-if (block_job_is_cancelled(job)) {
-block_job_event_cancelled(job);
-} else {
-const char *msg = NULL;
-if (job->ret < 0) {
-msg = strerror(-job->ret);
+
+/* Emit events only if we actually started */
+if (block_job_started(job)) {
+if (block_job_is_cancelled(job)) {
+block_job_event_cancelled(job);
+} else {
+const char *msg = NULL;
+if (job->ret < 0) {
+msg = strerror(-job->ret);
+}
+block_job_event_completed(job, msg);
 }
-block_job_event_completed(job, msg);
 }
 
 if (job->txn) {
@@ -363,7 +383,8 @@ void block_job_complete(BlockJob *job, Error **errp)
 {
 /* Should not be reachable via external interface for internal jobs */
 assert(job->id);
-if (job->pause_count || job->cancelled || !job->driver->complete) {
+if (job->pause_count || job->cancelled ||
+

[Qemu-block] [PATCH v3 1/6] blockjob: fix dead pointer in txn list

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Though it is not intended to be reached through normal circumstances,
if we do not gracefully deconstruct the transaction QLIST, we may wind
up with stale pointers in the list.

The rest of this series attempts to address the underlying issues,
but this should fix list inconsistencies.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Tested-by: John Snow 
Reviewed-by: John Snow 
[Rewrote commit message. --js]
Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 

Signed-off-by: John Snow 
---
 blockjob.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockjob.c b/blockjob.c
index 4aa14a4..4d0ef53 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -256,6 +256,7 @@ static void block_job_completed_single(BlockJob *job)
 }
 
 if (job->txn) {
+QLIST_REMOVE(job, txn_list);
 block_job_txn_unref(job->txn);
 }
 block_job_unref(job);
-- 
2.7.4

Re: [Qemu-block] [PULL v2 00/14] Block patches for 2.8

[Stefan Hajnoczi]

On Tue, Nov 01, 2016 at 08:50:57AM -0400, Jeff Cody wrote:
> The following changes since commit bf99fd3983d7185178a0f65ce29bb94b1aecaed1:
> 
>   Merge remote-tracking branch 'remotes/rth/tags/pull-sparc-20161031-2' into 
> staging (2016-11-01 11:21:02 +)
> 
> are available in the git repository at:
> 
>   g...@github.com:codyprime/qemu-kvm-jtc.git tags/block-pull-request

This pull request does not have a publicly-accessible URI.

Please adjust your .gitconfig:

[remote "public"]
url = git://github.com/codyprime/qemu.git
pushurl = g...@github.com:codyprime/qemu.git

Thanks,
Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0

[Kevin Wolf]

Am 02.11.2016 um 16:01 hat John Snow geschrieben:
> 
> 
> On 11/02/2016 09:33 AM, Kevin Wolf wrote:
> >Am 01.11.2016 um 04:16 hat John Snow geschrieben:
> >>Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
> >>NONDATA, but this introduced a regression for certain commands better
> >>described as CONDDATA. read_cd is such a command that both requires
> >>a non-zero BCL if a transfer size is set, but is perfectly content to
> >>accept a zero BCL if the transfer size is 0.
> >>
> >>This test adds a regression test for the case where BCL and nb_sectors
> >>are both 0.
> >>
> >>Flesh out the CDROM tests by:
> >>
> >>(1) Allowing the test to specify a BCL
> >>(2) Allowing the buffer comparison test to compare a 0-size buffer
> >>(3) Fix the BCL specification in libqos (It is LE, not BE)
> >>(4) Add a nice human-readable message for future SCSI command additions
> >>
> >>Signed-off-by: John Snow 
> >
> >>diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> >>index 5180d65..15fa888 100644
> >>--- a/tests/libqos/ahci.c
> >>+++ b/tests/libqos/ahci.c
> >>@@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
> >> return cmd;
> >> }
> >>
> >>-AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
> >>+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
> >> {
> >> AHCICommand *cmd = ahci_command_create(CMD_PACKET);
> >> cmd->atapi_cmd = g_malloc0(16);
> >> cmd->atapi_cmd[0] = scsi_cmd;
> >>-/* ATAPI needs a PIO transfer chunk size set inside of the LBA 
> >>registers.
> >>- * The block/sector size is a natural default. */
> >>-cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
> >>-cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
> >>-
> >>+stw_le_p(>fis.lba_lo[1], bcl);
> >> return cmd;
> >> }
> >
> >If I'm not mistaken, you're changing the endianness here, which seems
> >to be a silent bug fix.
> >
> >For some reason the test passes both ways. Does the actual value even
> >matter with AHCI, as long as it's non-zero? Do we end up with the same
> >result with BCL=0x0200 and BCL=0x0002, just that we split it into some
> >more iterations for the latter (or deeper recursion, actually)?
> >
> >Kevin
> >
> 
> Well, not silent, I did mention it in the cover letter. Your
> analysis of the mistake is correct. One way is just simply more
> iterations.

You mean I'm supposed to actually read commit messages...?

Sorry for the noise, looks good then.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Eric Blake]

On 11/02/2016 06:15 AM, Kevin Wolf wrote:
> Am 02.11.2016 um 11:52 hat Hao QingFeng geschrieben:
>> Sorry for a bit late response. The function looks fine but just some
>> doubt on g_renew in this piece of code(and the legacy), does
>> g_renew(realloc) has much performance impact if it's call many times
>> since alloc and memory copy are both also run many times?  So how
>> about increase the memory for more instead of 1 each time?
>>
>> formats = g_renew(const char *, formats, count + 10);
>>
>> A new counter also needs to be introduced to record the space size.

You are correct that the naive code has O(n^2) complexity, but so does
your replacement.  The only way to get to O(n) amortized complexity is
to change count by a geometric scale (the simplest is doubling each time
you have to realloc, but even something like increasing by 50% any time
an increase is needed would also work).  So if it is ever worth tracking
allocation size to reduce reallocation costs, it is worth doing it right
by using geometric rather than linear growth.

> 
> This code is not on a hot path, so keeping the code simple and easy to
> maintain is preferrable to complicating it just so --help can save a few
> CPU cycles.

But Kevin makes a good point - for small values of n, the difference
between running a complex O(n) algorithm or a simpler naive O(n^2)
algorithm can actually be faster for the naive algorithm; complexity
alone is not necessarily a good measure of performance until you have
large enough n that it becomes the dominating factor.  And this
certainly counts as code that is not executed frequently, nor where we
expect to have so many distinct formats that n will ever grow large
enough that the O(n^2) nature of the algorithm is noticeable to the
humans reading the help output.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0

[John Snow]

On 11/02/2016 09:33 AM, Kevin Wolf wrote:

Am 01.11.2016 um 04:16 hat John Snow geschrieben:

Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
NONDATA, but this introduced a regression for certain commands better
described as CONDDATA. read_cd is such a command that both requires
a non-zero BCL if a transfer size is set, but is perfectly content to
accept a zero BCL if the transfer size is 0.

This test adds a regression test for the case where BCL and nb_sectors
are both 0.

Flesh out the CDROM tests by:

(1) Allowing the test to specify a BCL
(2) Allowing the buffer comparison test to compare a 0-size buffer
(3) Fix the BCL specification in libqos (It is LE, not BE)
(4) Add a nice human-readable message for future SCSI command additions

Signed-off-by: John Snow 



diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 5180d65..15fa888 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
 return cmd;
 }

-AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
 {
 AHCICommand *cmd = ahci_command_create(CMD_PACKET);
 cmd->atapi_cmd = g_malloc0(16);
 cmd->atapi_cmd[0] = scsi_cmd;
-/* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
- * The block/sector size is a natural default. */
-cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
-cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
-
+stw_le_p(>fis.lba_lo[1], bcl);
 return cmd;
 }


If I'm not mistaken, you're changing the endianness here, which seems
to be a silent bug fix.

For some reason the test passes both ways. Does the actual value even
matter with AHCI, as long as it's non-zero? Do we end up with the same
result with BCL=0x0200 and BCL=0x0002, just that we split it into some
more iterations for the latter (or deeper recursion, actually)?

Kevin



Well, not silent, I did mention it in the cover letter. Your analysis of 
the mistake is correct. One way is just simply more iterations.


--js

Re: [Qemu-block] [PATCH 00/19] block: Fix some filename generation issues

[Alberto Garcia]

On Tue 26 Apr 2016 11:31:59 PM CEST, Max Reitz wrote:

> There are some issues regarding filename generation right now:
>
> - You always get a JSON filename if you set even a single qcow2-specific
>   runtime options (as long as it does not have a dot in it, which is a
>   bug, too, but here it is working in our favor...). That is not nice
>   and actually breaks the usage of backing files with relative
>   filenames with such qcow2 BDS.
>
> - As hinted above, you cannot use relative backing filenames with BDS
>   that have a JSON filename only, even though qemu might be able to
>   obtain the directory name by walking through the BDS graph to the
>   protocol level.
>
> - Overriding the backing file at runtime should invalidate the filename
>   because it actually changes the BDS's data. Therefore, we need to
>   force a JSON filename in that case, containing the backing file
>   override.
>
> - Much of our code assumes paths never to exceed PATH_MAX in length.
>   This is wrong, at least because of JSON filenames. This should be
>   fixed wherever the opportunity arises.

Hi Max,

I'd like to retake the review of this series. It can be rebased easily
and it still seems to work fine. Shall I take a look at the patches as
they are now or shall I better wait for a new version?

Berto

Re: [Qemu-block] [PATCH 00/19] block: Fix some filename generation issues

[Max Reitz]

On 02.11.2016 16:00, Alberto Garcia wrote:
> On Tue 26 Apr 2016 11:31:59 PM CEST, Max Reitz wrote:
> 
>> There are some issues regarding filename generation right now:
>>
>> - You always get a JSON filename if you set even a single qcow2-specific
>>   runtime options (as long as it does not have a dot in it, which is a
>>   bug, too, but here it is working in our favor...). That is not nice
>>   and actually breaks the usage of backing files with relative
>>   filenames with such qcow2 BDS.
>>
>> - As hinted above, you cannot use relative backing filenames with BDS
>>   that have a JSON filename only, even though qemu might be able to
>>   obtain the directory name by walking through the BDS graph to the
>>   protocol level.
>>
>> - Overriding the backing file at runtime should invalidate the filename
>>   because it actually changes the BDS's data. Therefore, we need to
>>   force a JSON filename in that case, containing the backing file
>>   override.
>>
>> - Much of our code assumes paths never to exceed PATH_MAX in length.
>>   This is wrong, at least because of JSON filenames. This should be
>>   fixed wherever the opportunity arises.
> 
> Hi Max,
> 
> I'd like to retake the review of this series. It can be rebased easily
> and it still seems to work fine. Shall I take a look at the patches as
> they are now or shall I better wait for a new version?

Thanks! I think it would be better to wait for a new version, which I'll
get to hopefully sooner than later.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 0/3] atapi: classify read_cd as conditionally returning data

[Kevin Wolf]

Am 01.11.2016 um 04:16 hat John Snow geschrieben:
> v2:
>  - Actually applied the changes this time ...
>  - And added a test to the AHCI suite...
>  - ...Which revealed a few small issues in the suite.
> 
> The AHCI test should be sufficient in terms of general proof
> for ATAPI regardless of the HBA used.

As I commented, I think patch 3 includes a silent bug fix, so maybe
check whether you agree with my understanding of the code and consider
to make this more explicit. In any case, I think it's still correct:

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 3/3] ahci-test: test atapi read_cd with bcl, nb_sectors = 0

[Kevin Wolf]

Am 01.11.2016 um 04:16 hat John Snow geschrieben:
> Commit 9ef2e93f introduced the concept of tagging ATAPI commands as
> NONDATA, but this introduced a regression for certain commands better
> described as CONDDATA. read_cd is such a command that both requires
> a non-zero BCL if a transfer size is set, but is perfectly content to
> accept a zero BCL if the transfer size is 0.
> 
> This test adds a regression test for the case where BCL and nb_sectors
> are both 0.
> 
> Flesh out the CDROM tests by:
> 
> (1) Allowing the test to specify a BCL
> (2) Allowing the buffer comparison test to compare a 0-size buffer
> (3) Fix the BCL specification in libqos (It is LE, not BE)
> (4) Add a nice human-readable message for future SCSI command additions
> 
> Signed-off-by: John Snow 

> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index 5180d65..15fa888 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -864,16 +865,12 @@ AHCICommand *ahci_command_create(uint8_t command_name)
>  return cmd;
>  }
>  
> -AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
> +AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
>  {
>  AHCICommand *cmd = ahci_command_create(CMD_PACKET);
>  cmd->atapi_cmd = g_malloc0(16);
>  cmd->atapi_cmd[0] = scsi_cmd;
> -/* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
> - * The block/sector size is a natural default. */
> -cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
> -cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
> -
> +stw_le_p(>fis.lba_lo[1], bcl);
>  return cmd;
>  }

If I'm not mistaken, you're changing the endianness here, which seems
to be a silent bug fix.

For some reason the test passes both ways. Does the actual value even
matter with AHCI, as long as it's non-zero? Do we end up with the same
result with BCL=0x0200 and BCL=0x0002, just that we split it into some
more iterations for the latter (or deeper recursion, actually)?

Kevin

Re: [Qemu-block] ping Re: [PATCH 00/18] Dirty bitmaps postcopy migration

[Denis V. Lunev]

On 11/02/2016 02:13 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 25, 2016 at 04:04:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> ping
>>
>> For now there are some notes mostly about accessory patches. What about
>> migration itself? Is it ok? Has it a chance of being merged one day?
> This series mostly touches migration/ code.  Juan or DaveG?
yep. this is the problem :(

or they can just accept ;)

Re: [Qemu-block] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Hao QingFeng]

在 2016-11-02 19:15, Kevin Wolf 写道:

Am 02.11.2016 um 11:52 hat Hao QingFeng geschrieben:

Sorry for a bit late response. The function looks fine but just some
doubt on g_renew in this piece of code(and the legacy), does
g_renew(realloc) has much performance impact if it's call many times
since alloc and memory copy are both also run many times?  So how
about increase the memory for more instead of 1 each time?

formats = g_renew(const char *, formats, count + 10);

A new counter also needs to be introduced to record the space size.

This code is not on a hot path, so keeping the code simple and easy to
maintain is preferrable to complicating it just so --help can save a few
CPU cycles.

Got it. thanks!

Kevin



--
QingFeng Hao(Robin)

Re: [Qemu-block] [PATCH 2/3] block: Emit modules in bdrv_iterate_format()

[Kevin Wolf]

Am 02.11.2016 um 11:52 hat Hao QingFeng geschrieben:
> Sorry for a bit late response. The function looks fine but just some
> doubt on g_renew in this piece of code(and the legacy), does
> g_renew(realloc) has much performance impact if it's call many times
> since alloc and memory copy are both also run many times?  So how
> about increase the memory for more instead of 1 each time?
> 
> formats = g_renew(const char *, formats, count + 10);
> 
> A new counter also needs to be introduced to record the space size.

This code is not on a hot path, so keeping the code simple and easy to
maintain is preferrable to complicating it just so --help can save a few
CPU cycles.

Kevin

Re: [Qemu-block] [PATCH v2] block/ssh: Code cleanup for unused parameter

[Kevin Wolf]

Am 02.11.2016 um 11:53 hat Ashijeet Acharya geschrieben:
> This patch drops the unused parameter "BDRVSSHState" being passed into
> the ssh_config() function and does code cleanup. The unused parameter
> was introduced by the commit c322712.
> 
> Signed-off-by: Ashijeet Acharya 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] ping Re: [PATCH 00/18] Dirty bitmaps postcopy migration

[Stefan Hajnoczi]

On Tue, Oct 25, 2016 at 04:04:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> ping
> 
> For now there are some notes mostly about accessory patches. What about
> migration itself? Is it ok? Has it a chance of being merged one day?

This series mostly touches migration/ code.  Juan or DaveG?

> 16.08.2016 13:25, Vladimir Sementsov-Ogievskiy wrote:
> > v2:
> > some bugs fixed, iotests a bit changed and merged into one test.
> > based on block-next (https://github.com/XanClic/qemu/commits/block-next)
> > clone: tag postcopy-v2 from https://src.openvz.org/scm/~vsementsov/qemu.git
> > online: 
> > https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fpostcopy-v2
> > 
> > v1:
> > 
> > These series are derived from my 'Dirty bitmaps migration' series. The
> > core idea is switch to postcopy migration and drop usage of meta
> > bitmaps.
> > 
> > These patches provide dirty bitmap postcopy migration feature. Only
> > named dirty bitmaps are to be migrated. Migration may be enabled using
> > migration capabilities.
> > 
> > The overall method (thanks to John Snow):
> > 
> > 1. migrate bitmaps meta data in .save_live_setup
> > - create/find related bitmaps on target
> > - disable them
> > - create successors (anonimous children) only for enabled migrated
> >   bitmaps
> > 2. do nothing in precopy stage
> > 3. just before target vm start: enable successors, created in (1)
> > 4. migrate bitmap data
> > 5. reclaime bitmaps (merge successors to their parents)
> > 6. enable bitmaps (only bitmaps, which was enabled in source)
> > 
> > 
> > Some patches are unchnaged from (v7) of 'Dirty bitmaps migration'
> > (DBMv7). I've left Reviewed-by's for them, if you don't like it, say me
> > and I'll drop them in the following version.
> > 
> > So, relatively to last DBMv7:
> > 
> > 01-04: new patches, splitting common postcopy migration out of ram
> > postcopy migration
> > 05: equal to DBMv7.05
> > 06: new
> > 07: equal to DBMv7.06
> > 08: new
> > 09: equal to DBMv7.07
> > 10: new
> > 11: derived from DBMv7.08, see below
> > 12-15: equal to DBMv7.09-12
> > 16: derived from DBMv7.13
> > - switch from fifo to socket, as postcopy don't work with fifo
> >   for now
> > - change parameters: size, granularity, regions
> > - add time.sleep, to wait for postcopy migration phase (bad
> >   temporary solution.
> > - drop Reviewed-by
> > 17: new
> > 
> > 11: the core patch of the series, it is derived from
> > [DBMv7.08: migration: add migration_block-dirty-bitmap.c]
> > There are a lot of changes related to switching from precopy to
> > postcopy, but functions related to migration stream itself
> > (structs, send/load sequences) are mostly unchnaged.
> > 
> > So, changes, to switch from precopy to postcopy:
> > - removed all staff related to meta bitmaps and dirty phase!!!
> > - add dirty_bitmap_mig_enable_successors, and call it before
> >   target vm start in loadvm_postcopy_handle_run
> > - add enabled_bitmaps list of bitmaps for
> >   dirty_bitmap_mig_enable_successors
> > 
> > - enabled flag is send with start bitmap chunk instead of
> >   completion chunk
> > - sectors_per_chunk is calculated directly from CHUNK_SIZE, not
> >   using meta bitmap granularity
> > 
> > - dirty_bitmap_save_iterate: remove dirty_phase, move bulk_phase
> >   to postcopy stage
> > - dirty_bitmap_save_pending: remove dirty phase related pending,
> >   switch pending to non-postcopyable
> > - dirty_bitmap_load_start: get enabled flag and prepare
> >   successors for enabled bitmaps, also add them to
> >   enabled_bitmaps list
> > - dirty_bitmap_load_complete: for enabled bitmaps: merge them
> >   with successors and enable
> > 
> > - savevm handlers:
> >   * remove separate savevm_dirty_bitmap_live_iterate_handlers state
> > (it was bad idea, any way), and move its save_live_iterate to
> > savevm_dirty_bitmap_handlers
> >   * add is_active_iterate savevm handler, which allows iterations
> > only in postcopy stage (after stopping source vm)
> >   * add has_postcopy savevm handler. (ofcourse, just returning true)
> >   * use save_live_complete_postcopy instead of
> > save_live_complete_precopy
> > 
> > Other changes:
> > - some debug output changed
> > - remove MIN_LIVE_SIZE, is_live_iterative and related staff (it
> >   was needed to omit iterations if bitmap data is small, possibly
> >   this should be reimplemented)
> > 
> > Vladimir Sementsov-Ogievskiy (18):
> >migration: add has_postcopy savevm handler
> >migration: fix ram_save_pending
> >migration: split common postcopy out of ram postcopy
> >

[Qemu-block] [PATCH v2] block/ssh: Code cleanup for unused parameter

[Ashijeet Acharya]

This patch drops the unused parameter "BDRVSSHState" being passed into
the ssh_config() function and does code cleanup. The unused parameter
was introduced by the commit c322712.

Signed-off-by: Ashijeet Acharya 
---
Changes in v2:
- Modify the commit message and subject line
---
 block/ssh.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index ca071c5..15ed281 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -582,8 +582,7 @@ static bool ssh_process_legacy_socket_options(QDict 
*output_opts,
 return true;
 }
 
-static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
- Error **errp)
+static InetSocketAddress *ssh_config(QDict *options, Error **errp)
 {
 InetSocketAddress *inet = NULL;
 QDict *addr = NULL;
@@ -661,7 +660,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 }
 
 /* Pop the config into our state object, Exit if invalid */
-s->inet = ssh_config(s, options, errp);
+s->inet = ssh_config(options, errp);
 if (!s->inet) {
 ret = -EINVAL;
 goto err;
-- 
2.6.2

Re: [Qemu-block] [PATCH v3] block/nbd: Fix the leaked visitor

[Kevin Wolf]

Am 02.11.2016 um 11:40 hat Ashijeet Acharya geschrieben:
> This patch frees the leaked visitor in nbd_refresh_filename() and uses
> visit_free() to fix it. The leak was introduced by the commit 491d6c7.
> 
> Signed-off-by: Ashijeet Acharya 
> Reviewed-by: Eric Blake 

Thanks, applied to the block branch.

Kevin

[Qemu-block] [PATCH v3] block/nbd: Fix the leaked visitor

[Ashijeet Acharya]

This patch frees the leaked visitor in nbd_refresh_filename() and uses
visit_free() to fix it. The leak was introduced by the commit 491d6c7.

Signed-off-by: Ashijeet Acharya 
Reviewed-by: Eric Blake 
---
Changes in v3:
- Modify the Subject line
- Free the visitor immediately after visit_complete()
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index 8ef1438..d656eb1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -534,6 +534,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
QDict *options)
 ov = qobject_output_visitor_new(_qdict);
 visit_type_SocketAddress(ov, NULL, >saddr, _abort);
 visit_complete(ov, _qdict);
+visit_free(ov);
 assert(qobject_type(saddr_qdict) == QTYPE_QDICT);
 
 qdict_put_obj(opts, "server", saddr_qdict);
-- 
2.6.2

Re: [Qemu-block] [PATCH] block/ssh: Fix the regression of unused parameter

[Ashijeet Acharya]

On Wed, Nov 2, 2016 at 4:03 PM, Kevin Wolf  wrote:
> Am 31.10.2016 um 19:06 hat Ashijeet Acharya geschrieben:
>> This patch drops the unused parameter "BDRVSSHState" being passed into
>> the ssh_config() function and fixes the bug.
>>
>> Signed-off-by: Ashijeet Acharya 
>
> The patch looks good, but why do claim that this is a bug fix rather
> than just a code cleanup?

Okay, I will change the commit message and include the commit id in
this one too.

Ashijeet
>
> Kevin

Re: [Qemu-block] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Ashijeet Acharya]

On Wed, Nov 2, 2016 at 4:00 PM, Kevin Wolf  wrote:
> Am 02.11.2016 um 10:48 hat Ashijeet Acharya geschrieben:
>> This patch frees the leaked visitor in nbd_refresh_filename() and uses
>> visit_free() to fix it. The leak was introduced by the commit 491d6c7.
>>
>> Signed-off-by: Ashijeet Acharya 
>> Reviewed-by: Eric Blake 
>
> I don't think this would generally be called a regression, so I'd change
> the subject to just "nbd: Fix leaker visitor".
>
>> Changes in v2:
>> - Include the regression commit id in the commit message
>> ---
>>  block/nbd.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 8ef1438..ff9d01a 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
>> QDict *options)
>>  qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
>>  }
>>
>> +visit_free(ov);
>>  qdict_flatten(opts);
>>  bs->full_open_options = opts;
>>  }
>
> I would prefer freeing the visitor immediately after visit_complete() so
> that everything visitor related is in a single place.
>
> Both of these points don't make your patch wrong, of course, but would
> you mind changing them?

Sure, I will send v3 straight away.

Ashijeet

> Kevin

Re: [Qemu-block] [PATCH] block/ssh: Fix the regression of unused parameter

[Kevin Wolf]

Am 31.10.2016 um 19:06 hat Ashijeet Acharya geschrieben:
> This patch drops the unused parameter "BDRVSSHState" being passed into
> the ssh_config() function and fixes the bug.
> 
> Signed-off-by: Ashijeet Acharya 

The patch looks good, but why do claim that this is a bug fix rather
than just a code cleanup?

Kevin

Re: [Qemu-block] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Kevin Wolf]

Am 02.11.2016 um 10:48 hat Ashijeet Acharya geschrieben:
> This patch frees the leaked visitor in nbd_refresh_filename() and uses
> visit_free() to fix it. The leak was introduced by the commit 491d6c7.
> 
> Signed-off-by: Ashijeet Acharya 
> Reviewed-by: Eric Blake 

I don't think this would generally be called a regression, so I'd change
the subject to just "nbd: Fix leaker visitor".

> Changes in v2:
> - Include the regression commit id in the commit message
> ---
>  block/nbd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 8ef1438..ff9d01a 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
> QDict *options)
>  qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
>  }
>  
> +visit_free(ov);
>  qdict_flatten(opts);
>  bs->full_open_options = opts;
>  }

I would prefer freeing the visitor immediately after visit_complete() so
that everything visitor related is in a single place.

Both of these points don't make your patch wrong, of course, but would
you mind changing them?

Kevin

[Qemu-block] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Ashijeet Acharya]

This patch frees the leaked visitor in nbd_refresh_filename() and uses
visit_free() to fix it. The leak was introduced by the commit 491d6c7.

Signed-off-by: Ashijeet Acharya 
Reviewed-by: Eric Blake 
---
Changes in v2:
- Include the regression commit id in the commit message
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index 8ef1438..ff9d01a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
QDict *options)
 qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
 }
 
+visit_free(ov);
 qdict_flatten(opts);
 bs->full_open_options = opts;
 }
-- 
2.6.2

Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] migration: fix compiler warning on uninitialized variable

[Mark Cave-Ayland]

On 31/10/16 21:50, Jeff Cody wrote:

> Some older GCC versions (e.g. 4.4.7) report a warning on an
> uninitialized variable for 'request', even though all possible code
> paths that reference 'request' will be initialized.   To appease
> these versions, initialize the variable to 0.
> 
> Reported-by: Mark Cave-Ayland 
> Signed-off-by: Jeff Cody 
> ---
>  migration/colo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index e7224b8..93c85c5 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -439,7 +439,7 @@ void *colo_process_incoming_thread(void *opaque)
>  }
>  
>  while (mis->state == MIGRATION_STATUS_COLO) {
> -int request;
> +int request = 0;
>  
>  colo_wait_handle_message(mis->from_src_file, , _err);
>  if (local_err) {
> 

Hi Jeff,

I can confirm that this patch fixes the issue for me (and indeed I see
that Peter has already applied this to git master as a build fix).


Many thanks,

Mark.