[Qemu-devel] [PULL 1.3-rc1 0/5] Block patches for QEMU 1.3-rc1

[Qemu-devel] [PULL 1.3-rc1 0/5] Block patches for QEMU 1.3-rc1

[Stefan Hajnoczi]

The following changes since commit ecf51c9abe63eae282e5f912d9367ce75f36636a:

  tcg/ppc: Fix !softmmu case (2012-11-21 10:56:22 +0400)

are available in the git repository at:

  git://github.com/stefanha/qemu.git block

for you to fetch changes up to 72bcca73c7a67c8506fa737618861ad413dabf38:

  ide: Fix status register after short PRDs (2012-11-21 09:47:34 +0100)

----------------------------------------------------------------
Jeff Cody (1):
      block: add bdrv_reopen() support for raw hdev, floppy, and cdrom

Kevin Wolf (2):
      ide: Fix crash with too long PRD
      ide: Fix status register after short PRDs

Stefan Hajnoczi (1):
      vdi: don't override libuuid symbols

Stefan Priebe (1):
      use int64_t for return values from rbd instead of int

 block/raw-posix.c | 16 ++++++++++++++++
 block/rbd.c       |  4 ++--
 block/vdi.c       |  9 +++------
 hw/ide/core.c     | 13 +++++++++++++
 4 files changed, 34 insertions(+), 8 deletions(-)

-- 
1.8.0

[Qemu-devel] [PATCH 1/5] block: add bdrv_reopen() support for raw hdev, floppy, and cdrom

[Stefan Hajnoczi]

From: Jeff Cody <jcody@redhat.com>

For hdev, floppy, and cdrom, the reopen() handlers are the same as
for the file reopen handler.  For floppy and cdrom types, however,
we keep O_NONBLOCK, as in the _open function.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/raw-posix.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index f2f0404..550c81f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -333,6 +333,10 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     }
 #endif
 
+    if (s->type == FTYPE_FD || s->type == FTYPE_CD) {
+        raw_s->open_flags |= O_NONBLOCK;
+    }
+
     raw_parse_flags(state->flags, &raw_s->open_flags);
 
     raw_s->fd = -1;
@@ -1409,6 +1413,9 @@ static BlockDriver bdrv_host_device = {
     .bdrv_probe_device  = hdev_probe_device,
     .bdrv_file_open     = hdev_open,
     .bdrv_close         = raw_close,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit  = raw_reopen_commit,
+    .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
     .bdrv_has_zero_init = hdev_has_zero_init,
@@ -1530,6 +1537,9 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_probe_device	= floppy_probe_device,
     .bdrv_file_open     = floppy_open,
     .bdrv_close         = raw_close,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit  = raw_reopen_commit,
+    .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
     .bdrv_has_zero_init = hdev_has_zero_init,
@@ -1629,6 +1639,9 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_probe_device	= cdrom_probe_device,
     .bdrv_file_open     = cdrom_open,
     .bdrv_close         = raw_close,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit  = raw_reopen_commit,
+    .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
     .bdrv_has_zero_init = hdev_has_zero_init,
@@ -1748,6 +1761,9 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_probe_device	= cdrom_probe_device,
     .bdrv_file_open     = cdrom_open,
     .bdrv_close         = raw_close,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit  = raw_reopen_commit,
+    .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
     .bdrv_has_zero_init = hdev_has_zero_init,
-- 
1.8.0

[Qemu-devel] [PATCH 2/5] vdi: don't override libuuid symbols

[Stefan Hajnoczi]

It's poor symbol hygiene to provide a global symbols that collide with a
common library like libuuid.  If QEMU links against a shared library
that depends on uuid_generate() it can end up calling our stub version
of the function.

This exact scenario happened with GlusterFS libgfapi.so, which depends
on libglusterfs.so's uuid_generate().

Scope the uuid stubs for vdi.c only and avoid affecting other shared
objects.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vdi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index f35b12e..c8330b7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -60,9 +60,6 @@
 /* TODO: move uuid emulation to some central place in QEMU. */
 #include "sysemu.h"     /* UUID_FMT */
 typedef unsigned char uuid_t[16];
-void uuid_generate(uuid_t out);
-int uuid_is_null(const uuid_t uu);
-void uuid_unparse(const uuid_t uu, char *out);
 #endif
 
 /* Code configuration options. */
@@ -124,18 +121,18 @@ void uuid_unparse(const uuid_t uu, char *out);
 #define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED)
 
 #if !defined(CONFIG_UUID)
-void uuid_generate(uuid_t out)
+static inline void uuid_generate(uuid_t out)
 {
     memset(out, 0, sizeof(uuid_t));
 }
 
-int uuid_is_null(const uuid_t uu)
+static inline int uuid_is_null(const uuid_t uu)
 {
     uuid_t null_uuid = { 0 };
     return memcmp(uu, null_uuid, sizeof(uuid_t)) == 0;
 }
 
-void uuid_unparse(const uuid_t uu, char *out)
+static inline void uuid_unparse(const uuid_t uu, char *out)
 {
     snprintf(out, 37, UUID_FMT,
             uu[0], uu[1], uu[2], uu[3], uu[4], uu[5], uu[6], uu[7],
-- 
1.8.0

Re: [Qemu-devel] [PATCH 2/5] vdi: don't override libuuid symbols

[Stefan Weil]

Am 21.11.2012 11:12, schrieb Stefan Hajnoczi:
> It's poor symbol hygiene to provide a global symbols that collide with a
> common library like libuuid.  If QEMU links against a shared library
> that depends on uuid_generate() it can end up calling our stub version
> of the function.
>
> This exact scenario happened with GlusterFS libgfapi.so, which depends
> on libglusterfs.so's uuid_generate().
>
> Scope the uuid stubs for vdi.c only and avoid affecting other shared
> objects.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vdi.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>


Hi Stefan,

I'm not opposed to this patch but would like to better understand
the problem which you had with the old solution.

Does libglusterfs.so really have a uuid_generate? Why does it not
take the implementation from libuuid.so? Then QEMU could also
use libuuid.so, and there would be no problem at all.

I tried to reproduce the problem but could not find a libgfapi.so
in the Debian or Ubuntu package archives.

Regards
Stefan

Re: [Qemu-devel] [PATCH 2/5] vdi: don't override libuuid symbols

[Stefan Hajnoczi]

On Wed, Nov 21, 2012 at 08:25:24PM +0100, Stefan Weil wrote:
> Am 21.11.2012 11:12, schrieb Stefan Hajnoczi:
> >It's poor symbol hygiene to provide a global symbols that collide with a
> >common library like libuuid.  If QEMU links against a shared library
> >that depends on uuid_generate() it can end up calling our stub version
> >of the function.
> >
> >This exact scenario happened with GlusterFS libgfapi.so, which depends
> >on libglusterfs.so's uuid_generate().
> >
> >Scope the uuid stubs for vdi.c only and avoid affecting other shared
> >objects.
> >
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block/vdi.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> 
> 
> Hi Stefan,
> 
> I'm not opposed to this patch but would like to better understand
> the problem which you had with the old solution.
> 
> Does libglusterfs.so really have a uuid_generate? Why does it not
> take the implementation from libuuid.so? Then QEMU could also
> use libuuid.so, and there would be no problem at all.
> 
> I tried to reproduce the problem but could not find a libgfapi.so
> in the Debian or Ubuntu package archives.

libgfapi.so is only in glusterfs.git/master.  It has not yet been
available in a release.

The problem occurs when QEMU and GlusterFS are built on a system without
the libuuid.h development header.

QEMU's behavior in this situation is to build our own libuuid stubs in
block/vdi.c.  The symbols are global.

GlusterFS's behavior in this situation is to build with its own libuuid
implementation.  The symbols are global in libglusterfs.so.  The
libgfapi.so shared object links against libglusterfs.so and expects its
uuid_generate() symbol to be resolved.

The fun happens when using QEMU block/gluster.c and it calls into
libgfapi.so.  It seems QEMU's block/vdi.c libuuid stubs are bound and
GlusterFS is now calling QEMU's dummy code - uuid_generate() always
returns the zero UUID!

Both QEMU and GlusterFS should not provide global symbols for a common
library.

Stefan

[Qemu-devel] [PATCH 3/5] use int64_t for return values from rbd instead of int

[Stefan Hajnoczi]

From: Stefan Priebe <s.priebe@profihost.ag>

rbd / rados tends to return pretty often length of writes
or discarded blocks. These values might be bigger than int.

The steps to reproduce are:

  mkfs.xfs -f a whole device bigger than int in bytes. mkfs.xfs sends
  a discard. Important is that you use scsi-hd and set
  discard_granularity=512. Otherwise rbd disabled discard support.

Signed-off-by: Stefan Priebe <s.priebe@profihost.ag>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/rbd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 0aaacaf..f3becc7 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -69,7 +69,7 @@ typedef enum {
 typedef struct RBDAIOCB {
     BlockDriverAIOCB common;
     QEMUBH *bh;
-    int ret;
+    int64_t ret;
     QEMUIOVector *qiov;
     char *bounce;
     RBDAIOCmd cmd;
@@ -86,7 +86,7 @@ typedef struct RADOSCB {
     int done;
     int64_t size;
     char *buf;
-    int ret;
+    int64_t ret;
 } RADOSCB;
 
 #define RBD_FD_READ 0
-- 
1.8.0

[Qemu-devel] [PATCH 4/5] ide: Fix crash with too long PRD

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

Without this, s->nsector can become negative and badness happens (trying
to malloc huge amount of memory and glib calls abort())

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/ide/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7d6b0fa..c2ab787 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -579,6 +579,7 @@ void ide_dma_cb(void *opaque, int ret)
     IDEState *s = opaque;
     int n;
     int64_t sector_num;
+    bool stay_active = false;
 
     if (ret < 0) {
         int op = BM_STATUS_DMA_RETRY;
@@ -594,6 +595,14 @@ void ide_dma_cb(void *opaque, int ret)
     }
 
     n = s->io_buffer_size >> 9;
+    if (n > s->nsector) {
+        /* The PRDs were longer than needed for this request. Shorten them so
+         * we don't get a negative remainder. The Active bit must remain set
+         * after the request completes. */
+        n = s->nsector;
+        stay_active = true;
+    }
+
     sector_num = ide_get_sector(s);
     if (n > 0) {
         dma_buf_commit(s);
@@ -646,6 +655,9 @@ eot:
         bdrv_acct_done(s->bs, &s->acct);
     }
     ide_set_inactive(s);
+    if (stay_active) {
+        s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_DMAING);
+    }
 }
 
 static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
-- 
1.8.0

[Qemu-devel] [PATCH 5/5] ide: Fix status register after short PRDs

[Stefan Hajnoczi]

From: Kevin Wolf <kwolf@redhat.com>

When failing a request because the length of the regions described by
the PRDT was too short for the requested number of sectors, the IDE
emulation forgot to update the status register, so that the device would
keep the BSY flag set indefinitely.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c2ab787..8da894f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -625,6 +625,7 @@ void ide_dma_cb(void *opaque, int ret)
     if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) == 0) {
         /* The PRDs were too short. Reset the Active bit, but don't raise an
          * interrupt. */
+        s->status = READY_STAT | SEEK_STAT;
         goto eot;
     }
 
-- 
1.8.0

Re: [Qemu-devel] [PULL 1.3-rc1 0/5] Block patches for QEMU 1.3-rc1

[Anthony Liguori]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The following changes since commit ecf51c9abe63eae282e5f912d9367ce75f36636a:
>
>   tcg/ppc: Fix !softmmu case (2012-11-21 10:56:22 +0400)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git block

Pulled. Thanks.

Regards,

Anthony Liguori

>
> for you to fetch changes up to 72bcca73c7a67c8506fa737618861ad413dabf38:
>
>   ide: Fix status register after short PRDs (2012-11-21 09:47:34 +0100)
>
> ----------------------------------------------------------------
> Jeff Cody (1):
>       block: add bdrv_reopen() support for raw hdev, floppy, and cdrom
>
> Kevin Wolf (2):
>       ide: Fix crash with too long PRD
>       ide: Fix status register after short PRDs
>
> Stefan Hajnoczi (1):
>       vdi: don't override libuuid symbols
>
> Stefan Priebe (1):
>       use int64_t for return values from rbd instead of int
>
>  block/raw-posix.c | 16 ++++++++++++++++
>  block/rbd.c       |  4 ++--
>  block/vdi.c       |  9 +++------
>  hw/ide/core.c     | 13 +++++++++++++
>  4 files changed, 34 insertions(+), 8 deletions(-)
>
> -- 
> 1.8.0