Re: [Qemu-block] [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P . Berrangé]

On Mon, Nov 19, 2018 at 11:00:38AM -0600, Eric Blake wrote:
> On 11/19/18 4:37 AM, Daniel P. Berrangé wrote:
> 
> > > Actually, I tracked this message down to using socat (which actually
> > > connects and then abruptly exits) when probing whether the socket is up 
> > > and
> > > listening.  That is, the message is being produced as a side effect of
> > > nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG
> > > command we are interested in testing.
> 
> > > > This is the first use of socat in iotests.  Might not be the most
> > > > portable, but I don't know if I have better ideas.
> > > > nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free
> > > > ports, but I don't know if ss is any better than socat.
> > > 
> > > So, I'm planning to squash this in, to use ss instead of socat, as 
> > > follows:
> > 
> > Personally I prefer socat since it is more portable, per my previous
> > message.
> 
> socat is indeed probably more portable, but since tests 233 uses
> '_supported_os Linux', ss availability isn't a problem until a future user
> ports this test to non-Linux.  I'd like to patch qemu-nbd to NOT warn about
> a user that connects but does not consume data (the socat case, as well as
> port probes), but as that patch does not exist yet and -rc2 is getting
> close, I'll go ahead and send the pull request with ss instead of socat.

Yes, that's ok with me.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] [PATCH 1/6 for-3.1] nbd: fix whitespace in server error message

[Philippe Mathieu-Daudé]

On 16/11/18 17:01, Eric Blake wrote:

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:

A space was missing after the option number was printed:

   Option 0x8not permitted before TLS

becomes

   Option 0x8 not permitted before TLS

This fixes

   commit 3668328303429f3bc93ab3365c66331600b06a2d
   Author: Eric Blake 
   Date:   Fri Oct 14 13:33:09 2016 -0500

 nbd: Send message along with server NBD_REP_ERR errors

Signed-off-by: Daniel P. Berrangé 
---
  nbd/server.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4e8f5ae51b..12e8139f95 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient 
*client, uint16_t myflags,

  default:
  ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
-   "Option 0x%" PRIx32
+   "Option 0x%" PRIx32 " "
 "not permitted before TLS", option);


Visually, I'd include the space in the next line instead of on its own 


Agreed :)

Reviewed-by: Philippe Mathieu-Daudé 

(but that's aesthetics, not semantic). Agree that this is 3.1 material; 
I'll queue it up and send a PR on Monday.


Reviewed-by: Eric Blake 

[Qemu-block] When AioHandler->is_external=true?

[Dongli Zhang]

Hi,

Would you please help explain in which case AioHandler->is_external is true, and
when it is false?

I read about iothread and mainloop and I am little bit confused about it.

Thank you very much!

Dongli Zhang

Re: [Qemu-block] [Qemu-devel] [PATCH] migration/block-dirty-bitmap: Silence coverity CID 1390625

[Vladimir Sementsov-Ogievskiy]

16.11.2018 21:43, John Snow wrote:
> Coverity warns that backing_bs() could give us a NULL pointer, which
> we then use without checking that it isn't.
> 
> In our loop condition, we check bs && bs->drv as a point of habit, but
> by nature of the block graph, we cannot have null bs pointers here.
> 
> This loop skips only implicit nodes, which always have children, so
> this loop should never encounter a null value.

You mean, always have backing (not file for ex.)? Should we at least add a 
comment
near "bool implicit;" that the node must have backing..

Do we have filters, using 'file' child instead of backing, will we want to auto 
insert them, and therefore mark them with implicit=true?

And one more thing:
So, it's looks like a wrong way to search for all block-nodes, instead of 
looping through backing chain to the first not-implicit bds, we must 
recursively explore the whole block graph, to find _all_ the bitmaps.

> 
> Tighten the loop condition to coax Coverity into dropping
> its false positive.
> 
> Suggested-by: Stefan Hajnoczi 
> Signed-off-by: John Snow 


> ---
>   migration/block-dirty-bitmap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 5e90f44c2f..00c068fda3 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -284,7 +284,7 @@ static int init_dirty_bitmap_migration(void)
>   const char *drive_name = bdrv_get_device_or_node_name(bs);
>   
>   /* skip automatically inserted nodes */
> -while (bs && bs->drv && bs->implicit) {
> +while (bs->drv && bs->implicit) {
>   bs = backing_bs(bs);
>   }
>   
> 


-- 
Best regards,
Vladimir

[Qemu-block] [PATCH for-3.1? 0/3] strcpy: fix stringop-truncation warnings

[Marc-André Lureau]

Hi,

Some of those warnings have already been fixed, others have been
delayed as it could make sense to disable/ignoring the warning, or
write a custom strncpy() function.

In some cases where NUL-ending string is not mandatory (because the
string length is bound in some format or protocol), we can replace
strncpy() with qemu strpadcpy(), so that the destination string is
still NUL-ending in cases where the destination is larger than the
source string.

Some warnings can be shut up with assert() lines in some cases.

Marc-André Lureau (3):
  sheepdog: fix stringop-truncation warning
  migration: fix stringop-truncation warning
  acpi: fix stringop-truncation warnings

 block/sheepdog.c |  1 +
 hw/acpi/aml-build.c  |  6 --
 hw/acpi/core.c   | 13 +++--
 migration/global_state.c |  1 +
 4 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.19.1.708.g4ede3d42df

[Qemu-block] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning

[Marc-André Lureau]

It seems adding an assert is enough to silence GCC.
(sd_parse_snapid_or_tag() g_strlcpy() ensures that we don't get in
that situation)

~/src/qemu/block/sheepdog.c: In function 'find_vdi_name':
~/src/qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals 
destination size [-Werror=stringop-truncation]
 strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
 ^~
cc1: all warnings being treated as errors

Signed-off-by: Marc-André Lureau 
---
 block/sheepdog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0125df9d49..f8877b611d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1236,6 +1236,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char 
*filename,
  * don't want the send_req to read uninitialized data.
  */
 strncpy(buf, filename, SD_MAX_VDI_LEN);
+assert(strlen(tag) < SD_MAX_VDI_TAG_LEN);
 strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
 
 memset(&hdr, 0, sizeof(hdr));
-- 
2.19.1.708.g4ede3d42df

[Qemu-block] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings

[Marc-André Lureau]

Replace strcpy() that don't mind about having dest not ending with NUL
char by qemu strpadcpy().

Signed-off-by: Marc-André Lureau 
---
 hw/acpi/aml-build.c |  6 --
 hw/acpi/core.c  | 13 +++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736d..0a64a23e09 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -24,6 +24,7 @@
 #include "hw/acpi/aml-build.h"
 #include "qemu/bswap.h"
 #include "qemu/bitops.h"
+#include "qemu/cutils.h"
 #include "sysemu/numa.h"
 
 static GArray *build_alloc_array(void)
@@ -1532,13 +1533,14 @@ build_header(BIOSLinker *linker, GArray *table_data,
 h->revision = rev;
 
 if (oem_id) {
-strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
+strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, 0);
 } else {
 memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
 }
 
 if (oem_table_id) {
-strncpy((char *)h->oem_table_id, oem_table_id, 
sizeof(h->oem_table_id));
+strpadcpy((char *)h->oem_table_id, sizeof(h->oem_table_id),
+  oem_table_id, 0);
 } else {
 memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
 memcpy(h->oem_table_id + 4, sig, 4);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index aafdc61648..c1b4dfbfd9 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -31,6 +31,7 @@
 #include "qapi/qapi-visit-misc.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 
 struct acpi_table_header {
 uint16_t _length; /* our length, not actual part of the hdr */
@@ -181,7 +182,7 @@ static void acpi_table_install(const char unsigned *blob, 
size_t bloblen,
 ext_hdr->_length = cpu_to_le16(acpi_payload_size);
 
 if (hdrs->has_sig) {
-strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
+strpadcpy(ext_hdr->sig, sizeof ext_hdr->sig, hdrs->sig, 0);
 ++changed_fields;
 }
 
@@ -200,12 +201,12 @@ static void acpi_table_install(const char unsigned *blob, 
size_t bloblen,
 ext_hdr->checksum = 0;
 
 if (hdrs->has_oem_id) {
-strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
+strpadcpy(ext_hdr->oem_id, sizeof ext_hdr->oem_id, hdrs->oem_id, 0);
 ++changed_fields;
 }
 if (hdrs->has_oem_table_id) {
-strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
-sizeof ext_hdr->oem_table_id);
+strpadcpy(ext_hdr->oem_table_id, sizeof ext_hdr->oem_table_id,
+  hdrs->oem_table_id, 0);
 ++changed_fields;
 }
 if (hdrs->has_oem_rev) {
@@ -213,8 +214,8 @@ static void acpi_table_install(const char unsigned *blob, 
size_t bloblen,
 ++changed_fields;
 }
 if (hdrs->has_asl_compiler_id) {
-strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
-sizeof ext_hdr->asl_compiler_id);
+strpadcpy(ext_hdr->asl_compiler_id, sizeof ext_hdr->asl_compiler_id,
+  hdrs->asl_compiler_id, 0);
 ++changed_fields;
 }
 if (hdrs->has_asl_compiler_rev) {
-- 
2.19.1.708.g4ede3d42df

[Qemu-block] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning

[Marc-André Lureau]

Adding an assert is enough to silence GCC.

~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 
equals destination size [-Werror=stringop-truncation]
 strncpy((char *)global_state.runstate,
 ^~
state, sizeof(global_state.runstate));
~
cc1: all warnings being treated as errors

(alternatively, we could hard-code "running")

Signed-off-by: Marc-André Lureau 
---
 migration/global_state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/global_state.c b/migration/global_state.c
index 8e8ab5c51e..01805c567a 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -42,6 +42,7 @@ int global_state_store(void)
 void global_state_store_running(void)
 {
 const char *state = RunState_str(RUN_STATE_RUNNING);
+assert(strlen(state) < sizeof(global_state.runstate));
 strncpy((char *)global_state.runstate,
state, sizeof(global_state.runstate));
 }
-- 
2.19.1.708.g4ede3d42df

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning

[Eric Blake]

On 11/20/18 9:27 AM, Marc-André Lureau wrote:

It seems adding an assert is enough to silence GCC.
(sd_parse_snapid_or_tag() g_strlcpy() ensures that we don't get in
that situation)

~/src/qemu/block/sheepdog.c: In function 'find_vdi_name':
~/src/qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals 
destination size [-Werror=stringop-truncation]
  strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  ^~
cc1: all warnings being treated as errors

Signed-off-by: Marc-André Lureau 
---
  block/sheepdog.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Eric Blake 

and safe for 3.1 in my opinion

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

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings

[Eric Blake]

On 11/20/18 9:27 AM, Marc-André Lureau wrote:

Replace strcpy() that don't mind about having dest not ending with NUL
char by qemu strpadcpy().

Signed-off-by: Marc-André Lureau 
---
  hw/acpi/aml-build.c |  6 --
  hw/acpi/core.c  | 13 +++--
  2 files changed, 11 insertions(+), 8 deletions(-)



Reviewed-by: Eric Blake 

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

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning

[Eric Blake]

On 11/20/18 9:27 AM, Marc-André Lureau wrote:

Adding an assert is enough to silence GCC.

~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 
equals destination size [-Werror=stringop-truncation]
  strncpy((char *)global_state.runstate,
  ^~
 state, sizeof(global_state.runstate));
 ~
cc1: all warnings being treated as errors

(alternatively, we could hard-code "running")

Signed-off-by: Marc-André Lureau 
---
  migration/global_state.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Eric Blake 

I think this is safe for 3.1, but I know the migration code is 
particularly wary of assert()s, even when they are non-triggerable (a 
100-byte buffer at global_state.runstate is big enough for ALL of the 
run states, not just RUN_STATE_RUNNING).




diff --git a/migration/global_state.c b/migration/global_state.c
index 8e8ab5c51e..01805c567a 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -42,6 +42,7 @@ int global_state_store(void)
  void global_state_store_running(void)
  {
  const char *state = RunState_str(RUN_STATE_RUNNING);
+assert(strlen(state) < sizeof(global_state.runstate));
  strncpy((char *)global_state.runstate,
 state, sizeof(global_state.runstate));
  }



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

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning

[Dr. David Alan Gilbert]

* Eric Blake (ebl...@redhat.com) wrote:
> On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> > Adding an assert is enough to silence GCC.
> > 
> > ~/src/qemu/migration/global_state.c: In function 
> > 'global_state_store_running':
> > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 
> > 100 equals destination size [-Werror=stringop-truncation]
> >   strncpy((char *)global_state.runstate,
> >   ^~
> >  state, sizeof(global_state.runstate));
> >  ~
> > cc1: all warnings being treated as errors
> > 
> > (alternatively, we could hard-code "running")
> > 
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   migration/global_state.c | 1 +
> >   1 file changed, 1 insertion(+)
> 
> Reviewed-by: Eric Blake 
> 
> I think this is safe for 3.1, but I know the migration code is particularly
> wary of assert()s, even when they are non-triggerable (a 100-byte buffer at
> global_state.runstate is big enough for ALL of the run states, not just
> RUN_STATE_RUNNING).

That's OK; the universe would have to be particularly broken to trigger
that one, and it's in no way connected with any state, so it would
trigger on even the most basic tests.

However, I wonder if this fixes the problem on mingw builds - windows
asserts are not marked as noreturn.

Dave

> > 
> > diff --git a/migration/global_state.c b/migration/global_state.c
> > index 8e8ab5c51e..01805c567a 100644
> > --- a/migration/global_state.c
> > +++ b/migration/global_state.c
> > @@ -42,6 +42,7 @@ int global_state_store(void)
> >   void global_state_store_running(void)
> >   {
> >   const char *state = RunState_str(RUN_STATE_RUNNING);
> > +assert(strlen(state) < sizeof(global_state.runstate));
> >   strncpy((char *)global_state.runstate,
> >  state, sizeof(global_state.runstate));
> >   }
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-block] [PATCH 2/2] iotests: Replace assertEquals() with assertEqual()

[Kevin Wolf]

TestCase.assertEquals() is deprecated since Python 2.7. Recent Python
versions print a warning when the function is called, which makes test
cases fail.

Replace it with the preferred spelling assertEqual().

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/041| 6 +++---
 tests/qemu-iotests/118| 4 ++--
 tests/qemu-iotests/iotests.py | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 3615011d98..26bf1701eb 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -469,7 +469,7 @@ new_state = "1"
 self.assert_qmp(event, 'data/id', 'drive0')
 event = self.vm.get_qmp_event(wait=True)
 
-self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')
+self.assertEqual(event['event'], 'BLOCK_JOB_ERROR')
 self.assert_qmp(event, 'data/device', 'drive0')
 self.assert_qmp(event, 'data/operation', 'read')
 result = self.vm.qmp('query-block-jobs')
@@ -494,7 +494,7 @@ new_state = "1"
 self.assert_qmp(event, 'data/id', 'drive0')
 event = self.vm.get_qmp_event(wait=True)
 
-self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')
+self.assertEqual(event['event'], 'BLOCK_JOB_ERROR')
 self.assert_qmp(event, 'data/device', 'drive0')
 self.assert_qmp(event, 'data/operation', 'read')
 result = self.vm.qmp('query-block-jobs')
@@ -625,7 +625,7 @@ new_state = "1"
 self.assert_qmp(result, 'return', {})
 
 event = self.vm.event_wait(name='BLOCK_JOB_ERROR')
-self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')
+self.assertEqual(event['event'], 'BLOCK_JOB_ERROR')
 self.assert_qmp(event, 'data/device', 'drive0')
 self.assert_qmp(event, 'data/operation', 'write')
 result = self.vm.qmp('query-block-jobs')
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index c4f4c213ca..603e10e8a2 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -261,7 +261,7 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
 result = self.vm.qmp('blockdev-close-tray', id=self.device_name)
 # Should be a no-op
 self.assert_qmp(result, 'return', {})
-self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+self.assertEqual(self.vm.get_qmp_events(wait=False), [])
 
 def test_remove_on_closed(self):
 if not self.has_real_tray:
@@ -448,7 +448,7 @@ class TestChangeReadOnly(ChangeBaseClass):
read_only_mode='retain')
 self.assert_qmp(result, 'error/class', 'GenericError')
 
-self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+self.assertEqual(self.vm.get_qmp_events(wait=False), [])
 
 result = self.vm.qmp('query-block')
 self.assert_qmp(result, 'return[0]/inserted/ro', False)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 27bb2b600c..d537538ba0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -581,7 +581,7 @@ class QMPTestCase(unittest.TestCase):
 def wait_ready_and_cancel(self, drive='drive0'):
 self.wait_ready(drive=drive)
 event = self.cancel_and_wait(drive=drive)
-self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
+self.assertEqual(event['event'], 'BLOCK_JOB_COMPLETED')
 self.assert_qmp(event, 'data/type', 'mirror')
 self.assert_qmp(event, 'data/offset', event['data']['len'])
 
-- 
2.19.1

[Qemu-block] [PATCH 1/2] iotests: Replace time.clock() with Timeout

[Kevin Wolf]

time.clock() is deprecated since Python 3.3. Current Python versions
warn that the function will be removed in Python 3.8, and those warnings
make the test case 118 fail.

Replace it with the Timeout mechanism that is compatible with both
Python 2 and 3, and makes the code even a little nicer.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/118 | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index ff3b2ae3e7..c4f4c213ca 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -53,21 +53,17 @@ class ChangeBaseClass(iotests.QMPTestCase):
 if not self.has_real_tray:
 return
 
-timeout = time.clock() + 3
-while not self.has_opened and time.clock() < timeout:
-self.process_events()
-if not self.has_opened:
-self.fail('Timeout while waiting for the tray to open')
+with iotests.Timeout(3, 'Timeout while waiting for the tray to open'):
+while not self.has_opened:
+self.process_events()
 
 def wait_for_close(self):
 if not self.has_real_tray:
 return
 
-timeout = time.clock() + 3
-while not self.has_closed and time.clock() < timeout:
-self.process_events()
-if not self.has_opened:
-self.fail('Timeout while waiting for the tray to close')
+with iotests.Timeout(3, 'Timeout while waiting for the tray to close'):
+while not self.has_closed:
+self.process_events()
 
 class GeneralChangeTestsBaseClass(ChangeBaseClass):
 
-- 
2.19.1

[Qemu-block] [PATCH for-3.1 0/2] iotests: More Python 3 fixes

[Kevin Wolf]

Kevin Wolf (2):
  iotests: Replace time.clock() with Timeout
  iotests: Replace assertEquals() with assertEqual()

 tests/qemu-iotests/041|  6 +++---
 tests/qemu-iotests/118| 20 
 tests/qemu-iotests/iotests.py |  2 +-
 3 files changed, 12 insertions(+), 16 deletions(-)

-- 
2.19.1

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning

[Marc-André Lureau]

Hi

On Tue, Nov 20, 2018 at 9:22 PM Dr. David Alan Gilbert
 wrote:
>
> * Eric Blake (ebl...@redhat.com) wrote:
> > On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> > > Adding an assert is enough to silence GCC.
> > >
> > > ~/src/qemu/migration/global_state.c: In function 
> > > 'global_state_store_running':
> > > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified 
> > > bound 100 equals destination size [-Werror=stringop-truncation]
> > >   strncpy((char *)global_state.runstate,
> > >   ^~
> > >  state, sizeof(global_state.runstate));
> > >  ~
> > > cc1: all warnings being treated as errors
> > >
> > > (alternatively, we could hard-code "running")
> > >
> > > Signed-off-by: Marc-André Lureau 
> > > ---
> > >   migration/global_state.c | 1 +
> > >   1 file changed, 1 insertion(+)
> >
> > Reviewed-by: Eric Blake 
> >
> > I think this is safe for 3.1, but I know the migration code is particularly
> > wary of assert()s, even when they are non-triggerable (a 100-byte buffer at
> > global_state.runstate is big enough for ALL of the run states, not just
> > RUN_STATE_RUNNING).
>
> That's OK; the universe would have to be particularly broken to trigger
> that one, and it's in no way connected with any state, so it would
> trigger on even the most basic tests.
>
> However, I wonder if this fixes the problem on mingw builds - windows
> asserts are not marked as noreturn.

On f29, with mingw64-gcc-8.2.0-3.fc29.x86_64, it fixes the warning.

>
> Dave
>
> > >
> > > diff --git a/migration/global_state.c b/migration/global_state.c
> > > index 8e8ab5c51e..01805c567a 100644
> > > --- a/migration/global_state.c
> > > +++ b/migration/global_state.c
> > > @@ -42,6 +42,7 @@ int global_state_store(void)
> > >   void global_state_store_running(void)
> > >   {
> > >   const char *state = RunState_str(RUN_STATE_RUNNING);
> > > +assert(strlen(state) < sizeof(global_state.runstate));
> > >   strncpy((char *)global_state.runstate,
> > >  state, sizeof(global_state.runstate));
> > >   }
> > >
> >
> > --
> > Eric Blake, Principal Software Engineer
> > Red Hat, Inc.   +1-919-301-3266
> > Virtualization:  qemu.org | libvirt.org
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Hi
> 
> On Tue, Nov 20, 2018 at 9:22 PM Dr. David Alan Gilbert
>  wrote:
> >
> > * Eric Blake (ebl...@redhat.com) wrote:
> > > On 11/20/18 9:27 AM, Marc-André Lureau wrote:
> > > > Adding an assert is enough to silence GCC.
> > > >
> > > > ~/src/qemu/migration/global_state.c: In function 
> > > > 'global_state_store_running':
> > > > ~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified 
> > > > bound 100 equals destination size [-Werror=stringop-truncation]
> > > >   strncpy((char *)global_state.runstate,
> > > >   ^~
> > > >  state, sizeof(global_state.runstate));
> > > >  ~
> > > > cc1: all warnings being treated as errors
> > > >
> > > > (alternatively, we could hard-code "running")
> > > >
> > > > Signed-off-by: Marc-André Lureau 
> > > > ---
> > > >   migration/global_state.c | 1 +
> > > >   1 file changed, 1 insertion(+)
> > >
> > > Reviewed-by: Eric Blake 
> > >
> > > I think this is safe for 3.1, but I know the migration code is 
> > > particularly
> > > wary of assert()s, even when they are non-triggerable (a 100-byte buffer 
> > > at
> > > global_state.runstate is big enough for ALL of the run states, not just
> > > RUN_STATE_RUNNING).
> >
> > That's OK; the universe would have to be particularly broken to trigger
> > that one, and it's in no way connected with any state, so it would
> > trigger on even the most basic tests.
> >
> > However, I wonder if this fixes the problem on mingw builds - windows
> > asserts are not marked as noreturn.
> 
> On f29, with mingw64-gcc-8.2.0-3.fc29.x86_64, it fixes the warning.

OK, fine.


Reviewed-by: Dr. David Alan Gilbert 

Dave

> >
> > Dave
> >
> > > >
> > > > diff --git a/migration/global_state.c b/migration/global_state.c
> > > > index 8e8ab5c51e..01805c567a 100644
> > > > --- a/migration/global_state.c
> > > > +++ b/migration/global_state.c
> > > > @@ -42,6 +42,7 @@ int global_state_store(void)
> > > >   void global_state_store_running(void)
> > > >   {
> > > >   const char *state = RunState_str(RUN_STATE_RUNNING);
> > > > +assert(strlen(state) < sizeof(global_state.runstate));
> > > >   strncpy((char *)global_state.runstate,
> > > >  state, sizeof(global_state.runstate));
> > > >   }
> > > >
> > >
> > > --
> > > Eric Blake, Principal Software Engineer
> > > Red Hat, Inc.   +1-919-301-3266
> > > Virtualization:  qemu.org | libvirt.org
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Kevin Wolf]

Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
> Add tests that validate it is possible to connect to an NBD server
> running TLS mode. Also test mis-matched TLS vs non-TLS connections
> correctly fail.

> +echo
> +echo "== preparing TLS creds =="
> +
> +tls_x509_create_root_ca "ca1"
> +tls_x509_create_root_ca "ca2"
> +tls_x509_create_server "ca1" "server1"
> +tls_x509_create_client "ca1" "client1"
> +tls_x509_create_client "ca2" "client2"

Looks like we can't blindly assume that certtool exists. This test case
fails for me, starting with the following diff:

@@ -1,30 +1,21 @@
 QA output created by 233

 == preparing TLS creds ==
-Generating a self signed certificate...
-Generating a self signed certificate...
-Generating a signed certificate...
-Generating a signed certificate...
-Generating a signed certificate...
+./common.tls: line 71: certtool: command not found
+./common.tls: line 71: certtool: command not found
+./common.tls: line 98: certtool: command not found
+./common.tls: line 127: certtool: command not found
+./common.tls: line 127: certtool: command not found

Of course, after that everything else fails as well.

Kevin

Re: [Qemu-block] [PATCH] block/nvme: call blk_drain in NVMe reset code to avoid lockups

[Igor Druzhinin]

On 14/11/2018 17:42, Igor Druzhinin wrote:
> On 06/11/2018 12:16, Igor Druzhinin wrote:
>> When blk_flush called in NVMe reset path S/C queues are already freed
>> which means that re-entering AIO handling loop having some IO requests
>> unfinished will lockup or crash as their SG structures being potentially
>> reused. Call blk_drain before freeing the queues to avoid this nasty
>> scenario.
>>
>> Signed-off-by: Igor Druzhinin 
>> ---
>>  hw/block/nvme.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index fc7dacb..cdf836e 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -797,6 +797,8 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
>>  {
>>  int i;
>>  
>> +blk_drain(n->conf.blk);
>> +
>>  for (i = 0; i < n->num_queues; i++) {
>>  if (n->sq[i] != NULL) {
>>  nvme_free_sq(n->sq[i], n);
>>
> 
> ping?
> 

CC: Paolo Bonzini 

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/20/18 11:27 AM, Kevin Wolf wrote:

Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:

Add tests that validate it is possible to connect to an NBD server
running TLS mode. Also test mis-matched TLS vs non-TLS connections
correctly fail.



+echo
+echo "== preparing TLS creds =="
+
+tls_x509_create_root_ca "ca1"
+tls_x509_create_root_ca "ca2"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_x509_create_client "ca2" "client2"


Looks like we can't blindly assume that certtool exists. This test case
fails for me, starting with the following diff:


Looks like we'll need a followup patch to skip the test if certtool is 
not found. (I already did the same in common.nbd if 'ss' was not found; 
so it should be easy to copy...)


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

Re: [Qemu-block] [PATCH] block/nvme: call blk_drain in NVMe reset code to avoid lockups

[Paolo Bonzini]

On 20/11/18 18:31, Igor Druzhinin wrote:
> On 14/11/2018 17:42, Igor Druzhinin wrote:
>> On 06/11/2018 12:16, Igor Druzhinin wrote:
>>> When blk_flush called in NVMe reset path S/C queues are already freed
>>> which means that re-entering AIO handling loop having some IO requests
>>> unfinished will lockup or crash as their SG structures being potentially
>>> reused. Call blk_drain before freeing the queues to avoid this nasty
>>> scenario.
>>>
>>> Signed-off-by: Igor Druzhinin 
>>> ---
>>>  hw/block/nvme.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index fc7dacb..cdf836e 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -797,6 +797,8 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
>>>  {
>>>  int i;
>>>  
>>> +blk_drain(n->conf.blk);
>>> +
>>>  for (i = 0; i < n->num_queues; i++) {
>>>  if (n->sq[i] != NULL) {
>>>  nvme_free_sq(n->sq[i], n);
>>>
>>
>> ping?
>>
> 
> CC: Paolo Bonzini 
> 

Looks good to me.  Kevin, Max?

Paolo

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P . Berrangé]

On Tue, Nov 20, 2018 at 11:45:54AM -0600, Eric Blake wrote:
> On 11/20/18 11:27 AM, Kevin Wolf wrote:
> > Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
> > > Add tests that validate it is possible to connect to an NBD server
> > > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > > correctly fail.
> > 
> > > +echo
> > > +echo "== preparing TLS creds =="
> > > +
> > > +tls_x509_create_root_ca "ca1"
> > > +tls_x509_create_root_ca "ca2"
> > > +tls_x509_create_server "ca1" "server1"
> > > +tls_x509_create_client "ca1" "client1"
> > > +tls_x509_create_client "ca2" "client2"
> > 
> > Looks like we can't blindly assume that certtool exists. This test case
> > fails for me, starting with the following diff:
> 
> Looks like we'll need a followup patch to skip the test if certtool is not
> found. (I already did the same in common.nbd if 'ss' was not found; so it
> should be easy to copy...)

FWIW certtool is part of gnutls-utils and is available on every platform
that QEMU officially supports as a build target.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH v5 05/16] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Kevin Wolf]

Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Max Reitz 
> ---
>  block/stream.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 81a7ec8ece..262d280ccd 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -34,7 +34,7 @@ typedef struct StreamBlockJob {
>  BlockDriverState *base;
>  BlockdevOnError on_error;
>  char *backing_file_str;
> -int bs_flags;
> +bool bs_read_only;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> @@ -89,10 +89,10 @@ static void stream_clean(Job *job)
>  BlockDriverState *bs = blk_bs(bjob->blk);
>  
>  /* Reopen the image back in read-only mode if necessary */
> -if (s->bs_flags != bdrv_get_flags(bs)) {
> +if (s->bs_read_only) {
>  /* Give up write permissions before making it read-only */
>  blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
> -bdrv_reopen(bs, s->bs_flags, NULL);
> +bdrv_reopen_set_read_only(bs, true, NULL);
>  }
>  
>  g_free(s->backing_file_str);
> @@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>  {
>  StreamBlockJob *s;
>  BlockDriverState *iter;
> -int orig_bs_flags;
> +int bs_read_only;

bool certainly?

Kevin

Re: [Qemu-block] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Alberto Garcia]

On Tue 20 Nov 2018 07:21:21 PM CET, Kevin Wolf wrote:
> Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
>> Hi all,
>> 
>> when reopening a BlockDriverState using bdrv_reopen() and friends the
>> new options can be specified either with a QDict or with flags. Both
>> methods overlap and that makes the semantics and the implementation
>> unnecessarily complicated.
>> 
>> This series removes the 'flags' parameter from these functions, so
>> from now on all option changes must be specified using a QDict. Apart
>> from simplifying the API, a few bugs are fixed along the way. See the
>> individual patches for more details.
>> 
>> This was tested with the current master (460f0236c12a86a38692c12d9bf).
>
> Looks good to me, except for that one s/int/bool/ that I could do while
> applying.
>
> The only remaining question is - is all of this for 3.1, or which parts
> are? It's a long series, but there are also a few bug fixes in there.

It wasn't meant for 3.1, I don't think there's any urgency for that.
Patch 16 is perhaps worth applying now (and even patch 14), but the rest
can wait.

Berto

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/20/18 11:53 AM, Daniel P. Berrangé wrote:


+echo
+echo "== preparing TLS creds =="
+
+tls_x509_create_root_ca "ca1"
+tls_x509_create_root_ca "ca2"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_x509_create_client "ca2" "client2"


Looks like we can't blindly assume that certtool exists. This test case
fails for me, starting with the following diff:


Looks like we'll need a followup patch to skip the test if certtool is not
found. (I already did the same in common.nbd if 'ss' was not found; so it
should be easy to copy...)


FWIW certtool is part of gnutls-utils and is available on every platform
that QEMU officially supports as a build target.


Ported to the build target != installed on the build machine. The 
failure is happening on machines that do not have all the build 
prerequisites (since it should still be possible to configure qemu to 
build without TLS, right?)


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

Re: [Qemu-block] [PATCH v5 05/16] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Alberto Garcia]

On Tue 20 Nov 2018 07:00:29 PM CET, Kevin Wolf wrote:
>> @@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState 
>> *bs,
>>  {
>>  StreamBlockJob *s;
>>  BlockDriverState *iter;
>> -int orig_bs_flags;
>> +int bs_read_only;
>
> bool certainly?

Oops!

Berto

[Qemu-block] [PATCH] nvme: fix out-of-bounds access to the CMB

[Paolo Bonzini]

Because the CMB BAR has a min_access_size of 2, if you read the last
byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
error.  This is CVE-2018-16847.

Another way to fix this might be to register the CMB as a RAM memory
region, which would also be more efficient.  However, that might be a
change for big-endian machines; I didn't think this through and I don't
know how real hardware works.  Add a basic testcase for the CMB in case
somebody does this change later on.

Cc: Keith Busch 
Cc: qemu-block@nongnu.org
Reported-by: Li Qiang 
Reviewed-by: Li Qiang 
Tested-by: Li Qiang 
Signed-off-by: Paolo Bonzini 
---
 hw/block/nvme.c|  2 +-
 tests/Makefile.include |  2 +-
 tests/nvme-test.c  | 68 +++---
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d0226e7fdc..ef046bbc54 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1199,7 +1199,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
 .write = nvme_cmb_write,
 .endianness = DEVICE_LITTLE_ENDIAN,
 .impl = {
-.min_access_size = 2,
+.min_access_size = 1,
 .max_access_size = 8,
 },
 };
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 613242bc6e..fb0b449c02 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o
 tests/machine-none-test$(EXESUF): tests/machine-none-test.o
 tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-virtio-obj-y)
 tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
-tests/nvme-test$(EXESUF): tests/nvme-test.o
+tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y)
 tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o
 tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o
 tests/ac97-test$(EXESUF): tests/ac97-test.o
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index 7674a446e4..2700ba838a 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -8,25 +8,73 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "libqtest.h"
+#include "libqos/libqos-pc.h"
+
+static QOSState *qnvme_start(const char *extra_opts)
+{
+QOSState *qs;
+const char *arch = qtest_get_arch();
+const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
+  "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s";
+
+if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+qs = qtest_pc_boot(cmd, extra_opts ? : "");
+global_qtest = qs->qts;
+return qs;
+}
+
+g_printerr("nvme tests are only available on x86\n");
+exit(EXIT_FAILURE);
+}
+
+static void qnvme_stop(QOSState *qs)
+{
+qtest_shutdown(qs);
+}
 
-/* Tests only initialization so far. TODO: Replace with functional tests */
 static void nop(void)
 {
+QOSState *qs;
+
+qs = qnvme_start(NULL);
+qnvme_stop(qs);
 }
 
-int main(int argc, char **argv)
+static void nvmetest_cmb_test(void)
 {
-int ret;
+const int cmb_bar_size = 2 * MiB;
+QOSState *qs;
+QPCIDevice *pdev;
+QPCIBar bar;
 
-g_test_init(&argc, &argv, NULL);
-qtest_add_func("/nvme/nop", nop);
+qs = qnvme_start("-global nvme.cmb_size_mb=2");
+pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
+g_assert(pdev != NULL);
+
+qpci_device_enable(pdev);
+bar = qpci_iomap(pdev, 2, NULL);
+
+qpci_io_writel(pdev, bar, 0, 0xccbbaa99);
+g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99);
+g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99);
+
+/* Test partially out-of-bounds accesses.  */
+qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211);
+g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11);
+g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, 0x2211);
+g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 
0x44332211);
+g_free(pdev);
 
-qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
-"-device nvme,drive=drv0,serial=foo");
-ret = g_test_run();
+qnvme_stop(qs);
+}
 
-qtest_end();
+int main(int argc, char **argv)
+{
+g_test_init(&argc, &argv, NULL);
+qtest_add_func("/nvme/nop", nop);
+qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test);
 
-return ret;
+return g_test_run();
 }
-- 
2.19.1

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB

[Paolo Bonzini]

On 19/11/18 18:43, Kevin Wolf wrote:
> Am 19.11.2018 um 18:09 hat Paolo Bonzini geschrieben:
>> On 19/11/18 16:23, Mark Kanda wrote:
>>> For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as
>>> opposed to this one). Was this done in error?
>>
>> Probably.  Kevin, can you revert and apply this one instead?  I don't
>> care if 3.1 or 3.2, but the previous fix is pointless complication.
> 
> I was waiting for you to address Li Qiang's review comments before I
> apply it. I can revert the other one once this is ready.

Sorry, I forgot to send it.  Did it now.

> Anyway, that .min_access_size influences the accessible range feels
> weird to me. Is this really how it is meant to work? I expected this
> only to influence the allowed granularity of accesses, and that the
> maximum accessible offset of the memory region is size - access_size.
>> Does this mean that the size parameter of memory_region_init_io() really
> means we allow access to offsets from 0 to size + impl.min_access_size - 1?
> If so, this is very surprising and I wonder if this is really the only
> device that gets it wrong.

Usually the offset is a register, so an invalid value will simply be
ignored by the device or reported as a guest error.

> For nvme it doesn't matter much because it can trivially support
> single-byte accesses, so this change is correct and fixes the problem,
> but isn't the real bug in access_with_adjusted_size(), which should
> adjust the accessed range in a way that it doesn't exceed the size of
> the memory region?

Hmm, what's happening is complicated.  memory_access_size is clamping
the access size to 1 because impl.unaligned is false.  However,
access_with_adjusted_size is bringing it back to 2 because it does

access_size = MAX(MIN(size, access_size_max), access_size_min);

So we could do something like

diff --git a/exec.c b/exec.c
index bb6170dbff..f1437b2be6 100644
--- a/exec.c
+++ b/exec.c
@@ -3175,7 +3175,11 @@
 if (!mr->ops->impl.unaligned) {
 unsigned align_size_max = addr & -addr;
 if (align_size_max != 0 && align_size_max < access_size_max) {
-access_size_max = align_size_max;
+unsigned access_size_min = mr->ops->valid.min_access_size;
+if (access_size_min == 0) {
+access_size_min = 1;
+}
+access_size_max = MAX(min_access_size, align_size_max);
 }
 }

Then I think the access size would remain 2 and and
memory_region_access_valid would reject it as unaligned.  That would
avoid the bug, but then nvme should be setting valid.min_access_size and
the exec.c patch alone would not be enough.

> I'm not sure why impl.min_access_size was set to 2 in the first place,
> but was valid.min_access_size meant maybe? Though if I read the spec
> correctly, that one should be 4, not 2.

I don't see any requirement for the CMB (section 4.7 in my copy)?

Paolo

Re: [Qemu-block] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Kevin Wolf]

Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
> Hi all,
> 
> when reopening a BlockDriverState using bdrv_reopen() and friends the
> new options can be specified either with a QDict or with flags. Both
> methods overlap and that makes the semantics and the implementation
> unnecessarily complicated.
> 
> This series removes the 'flags' parameter from these functions, so
> from now on all option changes must be specified using a QDict. Apart
> from simplifying the API, a few bugs are fixed along the way. See the
> individual patches for more details.
> 
> This was tested with the current master (460f0236c12a86a38692c12d9bf).

Looks good to me, except for that one s/int/bool/ that I could do while
applying.

The only remaining question is - is all of this for 3.1, or which parts
are? It's a long series, but there are also a few bug fixes in there.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 1/3] sheepdog: fix stringop-truncation warning

[Philippe Mathieu-Daudé]

On 20/11/18 16:27, Marc-André Lureau wrote:

It seems adding an assert is enough to silence GCC.
(sd_parse_snapid_or_tag() g_strlcpy() ensures that we don't get in
that situation)

~/src/qemu/block/sheepdog.c: In function 'find_vdi_name':
~/src/qemu/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals 
destination size [-Werror=stringop-truncation]
  strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  ^~
cc1: all warnings being treated as errors



Fixes: https://bugs.launchpad.net/bugs/1803872


Signed-off-by: Marc-André Lureau 
---
  block/sheepdog.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0125df9d49..f8877b611d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1236,6 +1236,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char 
*filename,
   * don't want the send_req to read uninitialized data.
   */
  strncpy(buf, filename, SD_MAX_VDI_LEN);
+assert(strlen(tag) < SD_MAX_VDI_TAG_LEN);
  strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
  
  memset(&hdr, 0, sizeof(hdr));




I tried to fix this warning this way:

-char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
+struct {
+char vdi[SD_MAX_VDI_LEN];
+char vdi_tag[SD_MAX_VDI_TAG_LEN];
+} buf;

...

-strncpy(buf, filename, SD_MAX_VDI_LEN);
-strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
+strncpy(buf.vdi, filename, SD_MAX_VDI_LEN);
+strncpy(buf.vdi_tag, tag, SD_MAX_VDI_TAG_LEN);

but your patch is simpler.

Reviewed-by: Philippe Mathieu-Daudé 

Thanks,

Phil.

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 2/3] migration: fix stringop-truncation warning

[Philippe Mathieu-Daudé]

On 20/11/18 16:27, Marc-André Lureau wrote:

Adding an assert is enough to silence GCC.

~/src/qemu/migration/global_state.c: In function 'global_state_store_running':
~/src/qemu/migration/global_state.c:45:5: error: 'strncpy' specified bound 100 
equals destination size [-Werror=stringop-truncation]
  strncpy((char *)global_state.runstate,
  ^~
 state, sizeof(global_state.runstate));
 ~
cc1: all warnings being treated as errors

(alternatively, we could hard-code "running")

Signed-off-by: Marc-André Lureau 


Reviewed-by: Philippe Mathieu-Daudé 


---
  migration/global_state.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/migration/global_state.c b/migration/global_state.c
index 8e8ab5c51e..01805c567a 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -42,6 +42,7 @@ int global_state_store(void)
  void global_state_store_running(void)
  {
  const char *state = RunState_str(RUN_STATE_RUNNING);
+assert(strlen(state) < sizeof(global_state.runstate));
  strncpy((char *)global_state.runstate,
 state, sizeof(global_state.runstate));
  }

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.1? 3/3] acpi: fix stringop-truncation warnings

[Philippe Mathieu-Daudé]

On 20/11/18 16:27, Marc-André Lureau wrote:

Replace strcpy() that don't mind about having dest not ending with NUL
char by qemu strpadcpy().

Signed-off-by: Marc-André Lureau 
---
  hw/acpi/aml-build.c |  6 --
  hw/acpi/core.c  | 13 +++--
  2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736d..0a64a23e09 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -24,6 +24,7 @@
  #include "hw/acpi/aml-build.h"
  #include "qemu/bswap.h"
  #include "qemu/bitops.h"
+#include "qemu/cutils.h"
  #include "sysemu/numa.h"
  
  static GArray *build_alloc_array(void)

@@ -1532,13 +1533,14 @@ build_header(BIOSLinker *linker, GArray *table_data,
  h->revision = rev;
  
  if (oem_id) {

-strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
+strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, 0);
  } else {
  memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
  }
  
  if (oem_table_id) {

-strncpy((char *)h->oem_table_id, oem_table_id, 
sizeof(h->oem_table_id));
+strpadcpy((char *)h->oem_table_id, sizeof(h->oem_table_id),
+  oem_table_id, 0);
  } else {
  memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
  memcpy(h->oem_table_id + 4, sig, 4);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index aafdc61648..c1b4dfbfd9 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -31,6 +31,7 @@
  #include "qapi/qapi-visit-misc.h"
  #include "qemu/error-report.h"
  #include "qemu/option.h"
+#include "qemu/cutils.h"
  
  struct acpi_table_header {

  uint16_t _length; /* our length, not actual part of the hdr */
@@ -181,7 +182,7 @@ static void acpi_table_install(const char unsigned *blob, 
size_t bloblen,
  ext_hdr->_length = cpu_to_le16(acpi_payload_size);
  
  if (hdrs->has_sig) {

-strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
+strpadcpy(ext_hdr->sig, sizeof ext_hdr->sig, hdrs->sig, 0);
  ++changed_fields;
  }
  
@@ -200,12 +201,12 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,

  ext_hdr->checksum = 0;
  
  if (hdrs->has_oem_id) {

-strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
+strpadcpy(ext_hdr->oem_id, sizeof ext_hdr->oem_id, hdrs->oem_id, 0);
  ++changed_fields;
  }
  if (hdrs->has_oem_table_id) {
-strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
-sizeof ext_hdr->oem_table_id);
+strpadcpy(ext_hdr->oem_table_id, sizeof ext_hdr->oem_table_id,
+  hdrs->oem_table_id, 0);
  ++changed_fields;
  }
  if (hdrs->has_oem_rev) {
@@ -213,8 +214,8 @@ static void acpi_table_install(const char unsigned *blob, 
size_t bloblen,
  ++changed_fields;
  }
  if (hdrs->has_asl_compiler_id) {
-strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
-sizeof ext_hdr->asl_compiler_id);
+strpadcpy(ext_hdr->asl_compiler_id, sizeof ext_hdr->asl_compiler_id,
+  hdrs->asl_compiler_id, 0);
  ++changed_fields;
  }
  if (hdrs->has_asl_compiler_rev) {



Reviewed-by: Philippe Mathieu-Daudé 

Sounds like a good idea. However we don't this particular one for 3.1, 
does it silents a warning?


Regards,

Phil.

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] iotests: Replace time.clock() with Timeout

[Philippe Mathieu-Daudé]

On 20/11/18 18:22, Kevin Wolf wrote:

time.clock() is deprecated since Python 3.3. Current Python versions
warn that the function will be removed in Python 3.8, and those warnings
make the test case 118 fail.

Replace it with the Timeout mechanism that is compatible with both
Python 2 and 3, and makes the code even a little nicer.

Signed-off-by: Kevin Wolf 


Reviewed-by: Philippe Mathieu-Daudé 


---
  tests/qemu-iotests/118 | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index ff3b2ae3e7..c4f4c213ca 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -53,21 +53,17 @@ class ChangeBaseClass(iotests.QMPTestCase):
  if not self.has_real_tray:
  return
  
-timeout = time.clock() + 3

-while not self.has_opened and time.clock() < timeout:
-self.process_events()
-if not self.has_opened:
-self.fail('Timeout while waiting for the tray to open')
+with iotests.Timeout(3, 'Timeout while waiting for the tray to open'):
+while not self.has_opened:
+self.process_events()
  
  def wait_for_close(self):

  if not self.has_real_tray:
  return
  
-timeout = time.clock() + 3

-while not self.has_closed and time.clock() < timeout:
-self.process_events()
-if not self.has_opened:
-self.fail('Timeout while waiting for the tray to close')
+with iotests.Timeout(3, 'Timeout while waiting for the tray to close'):
+while not self.has_closed:
+self.process_events()
  
  class GeneralChangeTestsBaseClass(ChangeBaseClass):
  

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] iotests: Replace assertEquals() with assertEqual()

[Philippe Mathieu-Daudé]

On 20/11/18 18:22, Kevin Wolf wrote:

TestCase.assertEquals() is deprecated since Python 2.7. Recent Python
versions print a warning when the function is called, which makes test
cases fail.

Replace it with the preferred spelling assertEqual().

Signed-off-by: Kevin Wolf 


Reviewed-by: Philippe Mathieu-Daudé 


---
  tests/qemu-iotests/041| 6 +++---
  tests/qemu-iotests/118| 4 ++--
  tests/qemu-iotests/iotests.py | 2 +-
  3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 3615011d98..26bf1701eb 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -469,7 +469,7 @@ new_state = "1"
  self.assert_qmp(event, 'data/id', 'drive0')
  event = self.vm.get_qmp_event(wait=True)
  
-self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')

+self.assertEqual(event['event'], 'BLOCK_JOB_ERROR')
  self.assert_qmp(event, 'data/device', 'drive0')
  self.assert_qmp(event, 'data/operation', 'read')
  result = self.vm.qmp('query-block-jobs')
@@ -494,7 +494,7 @@ new_state = "1"
  self.assert_qmp(event, 'data/id', 'drive0')
  event = self.vm.get_qmp_event(wait=True)
  
-self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')

+self.assertEqual(event['event'], 'BLOCK_JOB_ERROR')
  self.assert_qmp(event, 'data/device', 'drive0')
  self.assert_qmp(event, 'data/operation', 'read')
  result = self.vm.qmp('query-block-jobs')
@@ -625,7 +625,7 @@ new_state = "1"
  self.assert_qmp(result, 'return', {})
  
  event = self.vm.event_wait(name='BLOCK_JOB_ERROR')

-self.assertEquals(event['event'], 'BLOCK_JOB_ERROR')
+self.assertEqual(event['event'], 'BLOCK_JOB_ERROR')
  self.assert_qmp(event, 'data/device', 'drive0')
  self.assert_qmp(event, 'data/operation', 'write')
  result = self.vm.qmp('query-block-jobs')
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index c4f4c213ca..603e10e8a2 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -261,7 +261,7 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
  result = self.vm.qmp('blockdev-close-tray', id=self.device_name)
  # Should be a no-op
  self.assert_qmp(result, 'return', {})
-self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+self.assertEqual(self.vm.get_qmp_events(wait=False), [])
  
  def test_remove_on_closed(self):

  if not self.has_real_tray:
@@ -448,7 +448,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 
read_only_mode='retain')
  self.assert_qmp(result, 'error/class', 'GenericError')
  
-self.assertEquals(self.vm.get_qmp_events(wait=False), [])

+self.assertEqual(self.vm.get_qmp_events(wait=False), [])
  
  result = self.vm.qmp('query-block')

  self.assert_qmp(result, 'return[0]/inserted/ro', False)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 27bb2b600c..d537538ba0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -581,7 +581,7 @@ class QMPTestCase(unittest.TestCase):
  def wait_ready_and_cancel(self, drive='drive0'):
  self.wait_ready(drive=drive)
  event = self.cancel_and_wait(drive=drive)
-self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
+self.assertEqual(event['event'], 'BLOCK_JOB_COMPLETED')
  self.assert_qmp(event, 'data/type', 'mirror')
  self.assert_qmp(event, 'data/offset', event['data']['len'])
  

Re: [Qemu-block] [Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[Philippe Mathieu-Daudé]

On 20/11/18 19:41, Paolo Bonzini wrote:

Because the CMB BAR has a min_access_size of 2, if you read the last
byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
error.  This is CVE-2018-16847.

Another way to fix this might be to register the CMB as a RAM memory
region, which would also be more efficient.  However, that might be a
change for big-endian machines; I didn't think this through and I don't
know how real hardware works.  Add a basic testcase for the CMB in case
somebody does this change later on.

Cc: Keith Busch 
Cc: qemu-block@nongnu.org
Reported-by: Li Qiang 
Reviewed-by: Li Qiang 
Tested-by: Li Qiang 
Signed-off-by: Paolo Bonzini 
---
  hw/block/nvme.c|  2 +-
  tests/Makefile.include |  2 +-
  tests/nvme-test.c  | 68 +++---
  3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d0226e7fdc..ef046bbc54 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1199,7 +1199,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
  .write = nvme_cmb_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
  .impl = {
-.min_access_size = 2,
+.min_access_size = 1,
  .max_access_size = 8,
  },
  };
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 613242bc6e..fb0b449c02 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o
  tests/machine-none-test$(EXESUF): tests/machine-none-test.o
  tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-virtio-obj-y)
  tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
-tests/nvme-test$(EXESUF): tests/nvme-test.o
+tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y)
  tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o
  tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o
  tests/ac97-test$(EXESUF): tests/ac97-test.o
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index 7674a446e4..2700ba838a 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -8,25 +8,73 @@
   */
  
  #include "qemu/osdep.h"

+#include "qemu/units.h"
  #include "libqtest.h"
+#include "libqos/libqos-pc.h"
+
+static QOSState *qnvme_start(const char *extra_opts)
+{
+QOSState *qs;
+const char *arch = qtest_get_arch();
+const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
+  "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s";
+
+if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+qs = qtest_pc_boot(cmd, extra_opts ? : "");
+global_qtest = qs->qts;
+return qs;
+}
+
+g_printerr("nvme tests are only available on x86\n");
+exit(EXIT_FAILURE);
+}
+
+static void qnvme_stop(QOSState *qs)
+{
+qtest_shutdown(qs);
+}
  
-/* Tests only initialization so far. TODO: Replace with functional tests */

  static void nop(void)
  {
+QOSState *qs;
+
+qs = qnvme_start(NULL);
+qnvme_stop(qs);
  }
  
-int main(int argc, char **argv)

+static void nvmetest_cmb_test(void)
  {
-int ret;
+const int cmb_bar_size = 2 * MiB;
+QOSState *qs;
+QPCIDevice *pdev;
+QPCIBar bar;
  
-g_test_init(&argc, &argv, NULL);

-qtest_add_func("/nvme/nop", nop);
+qs = qnvme_start("-global nvme.cmb_size_mb=2");
+pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
+g_assert(pdev != NULL);
+
+qpci_device_enable(pdev);
+bar = qpci_iomap(pdev, 2, NULL);
+
+qpci_io_writel(pdev, bar, 0, 0xccbbaa99);
+g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99);
+g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99);
+
+/* Test partially out-of-bounds accesses.  */
+qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211);
+g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11);
+g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, 0x2211);
+g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 
0x44332211);
+g_free(pdev);
  
-qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "

-"-device nvme,drive=drv0,serial=foo");
-ret = g_test_run();
+qnvme_stop(qs);
+}
  
-qtest_end();

+int main(int argc, char **argv)
+{
+g_test_init(&argc, &argv, NULL);
+qtest_add_func("/nvme/nop", nop);
+qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test);
  
-return ret;

+return g_test_run();
  }



TEST: tests/nvme-test... (pid=29324)
  /x86_64/nvme/nop:  OK
  /x86_64/nvme/cmb_test: **
ERROR:tests/nvme-test.c:65:nvmetest_cmb_test: assertion failed 
(qpci_io_readb(pdev, bar, cmb_bar_size - 1) == 0x11): (0 == 17)

FAIL

Nice!

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

Re: [Qemu-block] [PATCH 2/2] iotests: Replace assertEquals() with assertEqual()

[John Snow]

On 11/20/18 12:22 PM, Kevin Wolf wrote:
> TestCase.assertEquals() is deprecated since Python 2.7. Recent Python
> versions print a warning when the function is called, which makes test
> cases fail.
> 
> Replace it with the preferred spelling assertEqual().
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: John Snow 

Re: [Qemu-block] [PATCH 1/2] iotests: Replace time.clock() with Timeout

[John Snow]

On 11/20/18 12:22 PM, Kevin Wolf wrote:
> time.clock() is deprecated since Python 3.3. Current Python versions
> warn that the function will be removed in Python 3.8, and those warnings
> make the test case 118 fail.
> 
> Replace it with the Timeout mechanism that is compatible with both
> Python 2 and 3, and makes the code even a little nicer.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/118 | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
> index ff3b2ae3e7..c4f4c213ca 100755
> --- a/tests/qemu-iotests/118
> +++ b/tests/qemu-iotests/118
> @@ -53,21 +53,17 @@ class ChangeBaseClass(iotests.QMPTestCase):
>  if not self.has_real_tray:
>  return
>  
> -timeout = time.clock() + 3
> -while not self.has_opened and time.clock() < timeout:
> -self.process_events()
> -if not self.has_opened:
> -self.fail('Timeout while waiting for the tray to open')
> +with iotests.Timeout(3, 'Timeout while waiting for the tray to 
> open'):
> +while not self.has_opened:
> +self.process_events()
>  
>  def wait_for_close(self):
>  if not self.has_real_tray:
>  return
>  
> -timeout = time.clock() + 3
> -while not self.has_closed and time.clock() < timeout:
> -self.process_events()
> -if not self.has_opened:
> -self.fail('Timeout while waiting for the tray to close')
> +with iotests.Timeout(3, 'Timeout while waiting for the tray to 
> close'):
> +while not self.has_closed:
> +self.process_events()
>  
>  class GeneralChangeTestsBaseClass(ChangeBaseClass):
>  
> 

I love the way that reads. Very cool!

Reviewed-by: John Snow 

Re: [Qemu-block] [PATCH for-3.1 0/2] iotests: More Python 3 fixes

[Kevin Wolf]

Am 20.11.2018 um 18:22 hat Kevin Wolf geschrieben:
> Kevin Wolf (2):
>   iotests: Replace time.clock() with Timeout
>   iotests: Replace assertEquals() with assertEqual()

Thanks for the quick reviews, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Kevin Wolf]

Am 20.11.2018 um 19:22 hat Eric Blake geschrieben:
> On 11/20/18 11:53 AM, Daniel P. Berrangé wrote:
> 
> > > > > +echo
> > > > > +echo "== preparing TLS creds =="
> > > > > +
> > > > > +tls_x509_create_root_ca "ca1"
> > > > > +tls_x509_create_root_ca "ca2"
> > > > > +tls_x509_create_server "ca1" "server1"
> > > > > +tls_x509_create_client "ca1" "client1"
> > > > > +tls_x509_create_client "ca2" "client2"
> > > > 
> > > > Looks like we can't blindly assume that certtool exists. This test case
> > > > fails for me, starting with the following diff:
> > > 
> > > Looks like we'll need a followup patch to skip the test if certtool is not
> > > found. (I already did the same in common.nbd if 'ss' was not found; so it
> > > should be easy to copy...)
> > 
> > FWIW certtool is part of gnutls-utils and is available on every platform
> > that QEMU officially supports as a build target.
> 
> Ported to the build target != installed on the build machine. The failure is
> happening on machines that do not have all the build prerequisites (since it
> should still be possible to configure qemu to build without TLS, right?)

It happens on my laptop, and qemu builds just fine.

Kevin

[Qemu-block] [PATCH] iotests: Skip 233 if certtool not installed

[Eric Blake]

The use of TLS while building qemu is optional. While the
'certtool' binary should be available on every platform that
supports building against TLS, that does not imply that the
developer has installed it.  Make the test gracefully skip
in that case.

Reported-by: Kevin Wolf 
Signed-off-by: Eric Blake 
---

On Fedora, libvirt requires libtls-utils to be present, but not qemu.

I'm fine if Kevin wants to pick this up in a pull request related
to iotests in general; if not, I'll do a pull request through my
NBD tree in time for -rc3.

 tests/qemu-iotests/common.tls | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
index 39f17c1b999..eae81789bbc 100644
--- a/tests/qemu-iotests/common.tls
+++ b/tests/qemu-iotests/common.tls
@@ -31,6 +31,9 @@ tls_x509_cleanup()

 tls_x509_init()
 {
+(certtool --help) >/dev/null 2>&1 || \
+   _notrun "certtool utility not found, skipping test"
+
 mkdir -p "${tls_dir}"

 # use a fixed key so we don't waste system entropy on
-- 
2.17.2

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: Skip 233 if certtool not installed

[Eric Blake]

On 11/20/18 4:52 PM, Eric Blake wrote:

The use of TLS while building qemu is optional. While the
'certtool' binary should be available on every platform that
supports building against TLS, that does not imply that the
developer has installed it.  Make the test gracefully skip
in that case.

Reported-by: Kevin Wolf 
Signed-off-by: Eric Blake 
---

On Fedora, libvirt requires libtls-utils to be present, but not qemu.


Sorry for the typo; certtool is part of gnutls-utils



I'm fine if Kevin wants to pick this up in a pull request related
to iotests in general; if not, I'll do a pull request through my
NBD tree in time for -rc3.

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

Re: [Qemu-block] [PATCH] iotests: Skip 233 if certtool not installed

[John Snow]

On 11/20/18 5:52 PM, Eric Blake wrote:
> The use of TLS while building qemu is optional. While the
> 'certtool' binary should be available on every platform that
> supports building against TLS, that does not imply that the
> developer has installed it.  Make the test gracefully skip
> in that case.
> 
> Reported-by: Kevin Wolf 
> Signed-off-by: Eric Blake 

Reviewed-by: John Snow 

> ---
> 
> On Fedora, libvirt requires libtls-utils to be present, but not qemu.
> 
> I'm fine if Kevin wants to pick this up in a pull request related
> to iotests in general; if not, I'll do a pull request through my
> NBD tree in time for -rc3.
> 
>  tests/qemu-iotests/common.tls | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> index 39f17c1b999..eae81789bbc 100644
> --- a/tests/qemu-iotests/common.tls
> +++ b/tests/qemu-iotests/common.tls
> @@ -31,6 +31,9 @@ tls_x509_cleanup()
> 
>  tls_x509_init()
>  {
> +(certtool --help) >/dev/null 2>&1 || \
> + _notrun "certtool utility not found, skipping test"
> +

Or something wickedly foul has happened :)

>  mkdir -p "${tls_dir}"
> 
>  # use a fixed key so we don't waste system entropy on
> 

Re: [Qemu-block] [PATCH for-3.1] iotests: Enhance 223 to cover multiple bitmap granularities

[John Snow]

On 11/19/18 12:29 PM, Eric Blake wrote:
> Testing granularity at the same size as the cluster isn't quite
> as fun as what happens when it is larger or smaller.  This
> enhancement also shows that qemu's nbd server can server the
> same disk over multiple exports simultaneously.
> 
> Signed-off-by: Eric Blake 

Tested-by: John Snow 
Reviewed-by: John Snow 

Good enhancement.

> ---
> 
> Just a testsuite enhancement, so suitable for -rc2 if it gets a
> quick review. Otherwise, I will probably include it as part of
> a larger series I'm working on to make qemu-nbd handle misaligned
> images more sanely (that's a pre-existing problem in the 3.0 release,
> so it's not fixing a regression and may be invovled enough to slip
> into 4.0, depending on what the rest of my series looks like).
> 
>  tests/qemu-iotests/223 | 43 +++---
>  tests/qemu-iotests/223.out | 32 +---
>  2 files changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
> index 72419e03388..397b865d347 100755
> --- a/tests/qemu-iotests/223
> +++ b/tests/qemu-iotests/223
> @@ -57,10 +57,11 @@ run_qemu()
>  }
> 
>  echo
> -echo "=== Create partially sparse image, then add dirty bitmap ==="
> +echo "=== Create partially sparse image, then add dirty bitmaps ==="
>  echo
> 
> -_make_test_img 4M
> +# Two bitmaps, to contrast granularity issues
> +_make_test_img -o cluster_size=4k 4M
>  $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io
>  run_qemu <  { "execute": "qmp_capabilities" }
> @@ -78,7 +79,16 @@ run_qemu <"arguments": {
>  "node": "n",
>  "name": "b",
> -"persistent": true
> +"persistent": true,
> +"granularity": 65536
> +  }
> +}
> +{ "execute": "block-dirty-bitmap-add",
> +  "arguments": {
> +"node": "n",
> +"name": "b2",
> +"persistent": true,
> +"granularity": 512
>}
>  }
>  { "execute": "quit" }
> @@ -88,10 +98,11 @@ echo
>  echo "=== Write part of the file under active bitmap ==="
>  echo
> 
> -$QEMU_IO -c 'w -P 0x22 2M 2M' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c 'w -P 0x22 512 512' -c 'w -P 0x33 2M 2M' "$TEST_IMG" \
> +| _filter_qemu_io
> 
>  echo
> -echo "=== End dirty bitmap, and start serving image over NBD ==="
> +echo "=== End dirty bitmaps, and start serving image over NBD ==="
>  echo
> 
>  _launch_qemu 2> >(_filter_nbd)
> @@ -103,6 +114,8 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
>  "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
>  _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
>"arguments":{"node":"n", "name":"b"}}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
> +  "arguments":{"node":"n", "name":"b2"}}' "return"
>  _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
>"arguments":{"addr":{"type":"unix",
>  "data":{"path":"'"$TEST_DIR/nbd"'"' "return"
> @@ -110,26 +123,40 @@ _send_qemu_cmd $QEMU_HANDLE 
> '{"execute":"nbd-server-add",
>"arguments":{"device":"n"}}' "return"
>  _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
>"arguments":{"name":"n", "bitmap":"b"}}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
> +  "arguments":{"device":"n", "name":"n2"}}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
> +  "arguments":{"name":"n2", "bitmap":"b2"}}' "return"
> 
>  echo
> -echo "=== Contrast normal status with dirty-bitmap status ==="
> +echo "=== Contrast normal status to large granularity dirty-bitmap ==="
>  echo
> 
>  QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
>  IMG="driver=nbd,export=n,server.type=unix,server.path=$TEST_DIR/nbd"
> -$QEMU_IO -r -c 'r -P 0 0 1m' -c 'r -P 0x11 1m 1m' \
> -  -c 'r -P 0x22 2m 2m' --image-opts "$IMG" | _filter_qemu_io
> +$QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 
> 1m' \
> +  -c 'r -P 0x33 2m 2m' --image-opts "$IMG" | _filter_qemu_io
>  $QEMU_IMG map --output=json --image-opts \
>"$IMG" | _filter_qemu_img_map
>  $QEMU_IMG map --output=json --image-opts \
>"$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
> 
> +echo
> +echo "=== Contrast to small granularity dirty-bitmap ==="
> +echo
> +
> +IMG="driver=nbd,export=n2,server.type=unix,server.path=$TEST_DIR/nbd"
> +$QEMU_IMG map --output=json --image-opts \
> +  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
> +
>  echo
>  echo "=== End NBD server ==="
>  echo
> 
>  _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
>"arguments":{"name":"n"}}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
> +  "arguments":{"name":"n2"}}' "return"
>  _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
>  _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"
> 
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 33021c8e6a1..de417477de

Re: [Qemu-block] [PATCH for-next? 0/2] qemu-img: Minor fixes to an amend error path

[John Snow]

On 11/19/18 5:19 AM, Max Reitz wrote:
> One of the amend error paths has two issues that are fixed by this
> series.  Since they are relatively minor and have been present in 3.0
> already, I think there is no need to get them into 3.1.  OTOH they are
> bug fixes, so they could go into 3.1 if you, dear reader, insist.

I enjoy your use of "dear reader" in cover letters.

Not related to the above:

Reviewed-by: John Snow 

> 
> 
> Max Reitz (2):
>   qemu-img: Fix typo
>   qemu-img: Fix leak
> 
>  qemu-img.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Re: [Qemu-block] KVM Forum block no[td]es

[Vladimir Sementsov-Ogievskiy]

On 12.11.2018 16:10, Nir Soffer wrote:
On Mon, Nov 12, 2018 at 5:26 PM Max Reitz 
mailto:mre...@redhat.com>> wrote:
On 12.11.18 00:36, Nir Soffer wrote:
> On Mon, Nov 12, 2018 at 12:25 AM Max Reitz 
> mailto:mre...@redhat.com>
> >> wrote:
>
> This is what I’ve taken from two or three BoF-like get-togethers on
> blocky things.  Amendments are more than welcome, of course.
>
> ...
>
> Bitmaps
>
> ===
>
> (Got this section from sneaking into a BoF I wasn’t invited to.  Oh
> well.  Won’t hurt to include them here.)
>
> Currently, when dirty bitmaps are loaded, all IN_USE bitmaps are just
> not loaded at all and completely ignored.  That isn’t correct, though,
> they should either still be loaded (and automatically treated and
> written back as fully dirty), or at least qemu-img check should
> “repair” them (i.e. fully dirtying them).
>
>
> I'm not sure making bitmaps dirty is better.
>
> When bitmap is marked IN_USE, it means that we cannot use it for
> backup. Deleting the bitmap or making it as bad so it cannot be used
> for the next backup is the same. Making the bitmap as dirty will full
> the management layer that everything was fine when the next backup
> includes the entire disk. It is better to cause the next backup to fail
> in a verbose way, since the backup software can recover by doing
> a full backup.

But making the dirty bitmap fully dirty will automatically enforce a
full backup.

True, but is also hide the fact that we lost the bitmap. I think we should
start with the simple way of making it fail and let the rest of the stack
fallback to full backup.


I agree. Management tool should decide what to do if we loose a bitmap. Having 
absolutely dirty dirty-bitmap is actually useless, as it's equal to not having 
a bitmap.

Anyway, we can add some flags to qemu-img check, to fix IN_USE bitmaps somehow. 
I thing the most simple and useful option would be just remove IN_USE bitmaps.


> Sometimes qemu (running in a mode as bare as possible) is better than
> using qemu-img convert, for instance.  It gives you more control
> (through QMP; you get e.g. better progress reporting), you get all of
> the mirror optimizations (we do have optimizations for convert, too,
> but whether it’s any good to write the same (or different?)
> optimizations twice is another question), and you get a common
> interface for everything (online and offline).
> Note that besides a bare qemu we’ve also always wanted to convert as
> many qemu-img operations into frontends for block jobs as possible.
> We have only done this for commit, however, even though convert looked
> like basically the ideal target.  It was just too hard with too little
> apparent gain, like always (and convert supports additional features
> like concatenation which we don’t have in the runtime block layer
> yet).
>
>
> I'm not sure it is better to run qemu and use qemu-img as thin wrapper
> over qmp.
>
> For example, management system may ascociate qemu-img
> with a sanlock lease, and sanlock may try to terminate qemu-img when
> a lease is invalidated. In this case sanlock will succeed while qemu is till
> accessing storage it should not access.
> ...

The point was not to run two processes, but to link the necessary bits
of qemu into qemu-img and then use them inside the single qemu-img
process itself.  As hinted at above, we've been doing this for qemu-img
commit for quite some time; that command actually runs a block job under
the hood (inside of qemu-img).

Sounds great.


Original idea was not to use qemu-img at all, but only qemu..


Nir

Re: [Qemu-block] When AioHandler->is_external=true?

[Fam Zheng]

On Tue, 11/20 20:34, Dongli Zhang wrote:
> Hi,
> 
> Would you please help explain in which case AioHandler->is_external is true, 
> and
> when it is false?
> 
> I read about iothread and mainloop and I am little bit confused about it.

VirtIO's ioeventfd is an example of is_external == true. It means the events
handler on this fd may initiate more I/O, such as read/write on virtual storage
backend, so are specially taken care of at certain points when we won't want
more I/O requests to be processed, such as when a block job is completing, or in
the middle of a QMP transaction.

Fam