All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes
@ 2017-11-01 14:25 Ross Lagerwall
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Ross Lagerwall @ 2017-11-01 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Ross Lagerwall, Daniel P. Berrange

Hi,

Here is a bug fix with the use of QIOChannelFile and 2 bug fixes and an
improvement to implementation of QIOChannelFile.

Regards,
Ross Lagerwall

Ross Lagerwall (4):
  migration: Don't leak IO channels
  io: Fix QIOChannelFile when creating and opening read-write
  io: Don't call close multiple times in QIOChannelFile
  io: Add /dev/fdset/ support to QIOChannelFile

 include/io/channel-file.h    |  2 +-
 io/channel-file.c            | 11 ++++-------
 migration/savevm.c           |  2 ++
 tests/test-io-channel-file.c | 29 +++++++++++++++++++++++++----
 4 files changed, 32 insertions(+), 12 deletions(-)

-- 
2.9.5

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels
  2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
@ 2017-11-01 14:25 ` Ross Lagerwall
  2018-01-18 13:41   ` Ross Lagerwall
                     ` (2 more replies)
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 2/4] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
                   ` (4 subsequent siblings)
  5 siblings, 3 replies; 16+ messages in thread
From: Ross Lagerwall @ 2017-11-01 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Ross Lagerwall, Juan Quintela, Dr. David Alan Gilbert

Since qemu_fopen_channel_{in,out}put take references on the underlying
IO channels, make sure to release our references to them.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
New in v2.

 migration/savevm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 4a88228..87557dd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2259,6 +2259,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
     }
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
     f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
+    object_unref(OBJECT(ioc));
     ret = qemu_save_device_state(f);
     qemu_fclose(f);
     if (ret < 0) {
@@ -2292,6 +2293,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     }
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
     f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
+    object_unref(OBJECT(ioc));
 
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v2 2/4] io: Fix QIOChannelFile when creating and opening read-write
  2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
@ 2017-11-01 14:25 ` Ross Lagerwall
  2018-01-18 13:49   ` Daniel P. Berrange
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile Ross Lagerwall
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Ross Lagerwall @ 2017-11-01 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Ross Lagerwall, Daniel P. Berrange

The code wrongly passes the mode to open() only if O_WRONLY is set.
Instead, the mode should be passed when O_CREAT is set (or O_TMPFILE on
Linux). Fix this by always passing the mode since open() will correctly
ignore the mode if it is not needed. Add a testcase which exercises this
bug and also change the existing testcase to check that the mode of the
created file is correct.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed in v2:
* Separated from qemu_open() change.

 include/io/channel-file.h    |  2 +-
 io/channel-file.c            |  6 +-----
 tests/test-io-channel-file.c | 29 +++++++++++++++++++++++++----
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/include/io/channel-file.h b/include/io/channel-file.h
index 79245f1..ebfe54e 100644
--- a/include/io/channel-file.h
+++ b/include/io/channel-file.h
@@ -73,7 +73,7 @@ qio_channel_file_new_fd(int fd);
  * qio_channel_file_new_path:
  * @path: the file path
  * @flags: the open flags (O_RDONLY|O_WRONLY|O_RDWR, etc)
- * @mode: the file creation mode if O_WRONLY is set in @flags
+ * @mode: the file creation mode if O_CREAT is set in @flags
  * @errp: pointer to initialized error object
  *
  * Create a new IO channel object for a file represented
diff --git a/io/channel-file.c b/io/channel-file.c
index b383273..16bf7ed 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -50,11 +50,7 @@ qio_channel_file_new_path(const char *path,
 
     ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE));
 
-    if (flags & O_WRONLY) {
-        ioc->fd = open(path, flags, mode);
-    } else {
-        ioc->fd = open(path, flags);
-    }
+    ioc->fd = open(path, flags, mode);
     if (ioc->fd < 0) {
         object_unref(OBJECT(ioc));
         error_setg_errno(errp, errno,
diff --git a/tests/test-io-channel-file.c b/tests/test-io-channel-file.c
index 6bfede6..2e94f63 100644
--- a/tests/test-io-channel-file.c
+++ b/tests/test-io-channel-file.c
@@ -24,16 +24,21 @@
 #include "io-channel-helpers.h"
 #include "qapi/error.h"
 
-static void test_io_channel_file(void)
+#define TEST_FILE "tests/test-io-channel-file.txt"
+#define TEST_MASK 0600
+
+static void test_io_channel_file_helper(int flags)
 {
     QIOChannel *src, *dst;
     QIOChannelTest *test;
+    struct stat st;
+    mode_t mask;
+    int ret;
 
-#define TEST_FILE "tests/test-io-channel-file.txt"
     unlink(TEST_FILE);
     src = QIO_CHANNEL(qio_channel_file_new_path(
                           TEST_FILE,
-                          O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600,
+                          flags, TEST_MASK,
                           &error_abort));
     dst = QIO_CHANNEL(qio_channel_file_new_path(
                           TEST_FILE,
@@ -45,18 +50,33 @@ static void test_io_channel_file(void)
     qio_channel_test_run_reader(test, dst);
     qio_channel_test_validate(test);
 
+    /* Check that the requested mode took effect. */
+    mask = umask(0);
+    umask(mask);
+    ret = stat(TEST_FILE, &st);
+    g_assert_cmpint(ret, >, -1);
+    g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0777);
+
     unlink(TEST_FILE);
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 }
 
+static void test_io_channel_file(void)
+{
+    test_io_channel_file_helper(O_WRONLY | O_CREAT | O_TRUNC | O_BINARY);
+}
+
+static void test_io_channel_file_rdwr(void)
+{
+    test_io_channel_file_helper(O_RDWR | O_CREAT | O_TRUNC | O_BINARY);
+}
 
 static void test_io_channel_fd(void)
 {
     QIOChannel *ioc;
     int fd = -1;
 
-#define TEST_FILE "tests/test-io-channel-file.txt"
     fd = open(TEST_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);
     g_assert_cmpint(fd, >, -1);
 
@@ -114,6 +134,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     g_test_add_func("/io/channel/file", test_io_channel_file);
+    g_test_add_func("/io/channel/file/rdwr", test_io_channel_file_rdwr);
     g_test_add_func("/io/channel/file/fd", test_io_channel_fd);
 #ifndef _WIN32
     g_test_add_func("/io/channel/pipe/sync", test_io_channel_pipe_sync);
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile
  2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 2/4] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
@ 2017-11-01 14:25 ` Ross Lagerwall
  2017-11-01 18:11   ` Marc-André Lureau
  2018-01-18 13:50   ` Daniel P. Berrange
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile Ross Lagerwall
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Ross Lagerwall @ 2017-11-01 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Ross Lagerwall, Daniel P. Berrange

If the file descriptor underlying QIOChannelFile is closed in the
io_close() method, don't close it again in the finalize() method since
the file descriptor number may have been reused in the meantime.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
New in v2.

 io/channel-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/io/channel-file.c b/io/channel-file.c
index 16bf7ed..1f2f710 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -178,6 +178,7 @@ static int qio_channel_file_close(QIOChannel *ioc,
                          "Unable to close file");
         return -1;
     }
+    fioc->fd = -1;
     return 0;
 }
 
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile
  2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
                   ` (2 preceding siblings ...)
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile Ross Lagerwall
@ 2017-11-01 14:25 ` Ross Lagerwall
  2017-11-01 18:12   ` Marc-André Lureau
  2018-01-18 13:50   ` Daniel P. Berrange
  2018-01-18 13:40 ` [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
  2018-01-18 13:51 ` Daniel P. Berrange
  5 siblings, 2 replies; 16+ messages in thread
From: Ross Lagerwall @ 2017-11-01 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Ross Lagerwall, Daniel P. Berrange

Add /dev/fdset/ support to QIOChannelFile by calling qemu_open() instead
of open() and qemu_close() instead of close(). There is a subtle
semantic change since qemu_open() automatically sets O_CLOEXEC, but this
doesn't affect any of the users of the function.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changed in v2:
* Split into separate patch.

 io/channel-file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io/channel-file.c b/io/channel-file.c
index 1f2f710..db948ab 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -50,7 +50,7 @@ qio_channel_file_new_path(const char *path,
 
     ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE));
 
-    ioc->fd = open(path, flags, mode);
+    ioc->fd = qemu_open(path, flags, mode);
     if (ioc->fd < 0) {
         object_unref(OBJECT(ioc));
         error_setg_errno(errp, errno,
@@ -74,7 +74,7 @@ static void qio_channel_file_finalize(Object *obj)
 {
     QIOChannelFile *ioc = QIO_CHANNEL_FILE(obj);
     if (ioc->fd != -1) {
-        close(ioc->fd);
+        qemu_close(ioc->fd);
         ioc->fd = -1;
     }
 }
@@ -173,7 +173,7 @@ static int qio_channel_file_close(QIOChannel *ioc,
 {
     QIOChannelFile *fioc = QIO_CHANNEL_FILE(ioc);
 
-    if (close(fioc->fd) < 0) {
+    if (qemu_close(fioc->fd) < 0) {
         error_setg_errno(errp, errno,
                          "Unable to close file");
         return -1;
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile Ross Lagerwall
@ 2017-11-01 18:11   ` Marc-André Lureau
  2018-01-18 13:50   ` Daniel P. Berrange
  1 sibling, 0 replies; 16+ messages in thread
From: Marc-André Lureau @ 2017-11-01 18:11 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: QEMU

On Wed, Nov 1, 2017 at 3:25 PM, Ross Lagerwall
<ross.lagerwall@citrix.com> wrote:
> If the file descriptor underlying QIOChannelFile is closed in the
> io_close() method, don't close it again in the finalize() method since
> the file descriptor number may have been reused in the meantime.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
> New in v2.
>
>  io/channel-file.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/io/channel-file.c b/io/channel-file.c
> index 16bf7ed..1f2f710 100644
> --- a/io/channel-file.c
> +++ b/io/channel-file.c
> @@ -178,6 +178,7 @@ static int qio_channel_file_close(QIOChannel *ioc,
>                           "Unable to close file");
>          return -1;
>      }
> +    fioc->fd = -1;
>      return 0;
>  }
>
> --
> 2.9.5
>
>



-- 
Marc-André Lureau

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile Ross Lagerwall
@ 2017-11-01 18:12   ` Marc-André Lureau
  2018-01-18 13:50   ` Daniel P. Berrange
  1 sibling, 0 replies; 16+ messages in thread
From: Marc-André Lureau @ 2017-11-01 18:12 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: QEMU

On Wed, Nov 1, 2017 at 3:25 PM, Ross Lagerwall
<ross.lagerwall@citrix.com> wrote:
> Add /dev/fdset/ support to QIOChannelFile by calling qemu_open() instead
> of open() and qemu_close() instead of close(). There is a subtle
> semantic change since qemu_open() automatically sets O_CLOEXEC, but this
> doesn't affect any of the users of the function.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
> Changed in v2:
> * Split into separate patch.
>
>  io/channel-file.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/io/channel-file.c b/io/channel-file.c
> index 1f2f710..db948ab 100644
> --- a/io/channel-file.c
> +++ b/io/channel-file.c
> @@ -50,7 +50,7 @@ qio_channel_file_new_path(const char *path,
>
>      ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE));
>
> -    ioc->fd = open(path, flags, mode);
> +    ioc->fd = qemu_open(path, flags, mode);
>      if (ioc->fd < 0) {
>          object_unref(OBJECT(ioc));
>          error_setg_errno(errp, errno,
> @@ -74,7 +74,7 @@ static void qio_channel_file_finalize(Object *obj)
>  {
>      QIOChannelFile *ioc = QIO_CHANNEL_FILE(obj);
>      if (ioc->fd != -1) {
> -        close(ioc->fd);
> +        qemu_close(ioc->fd);
>          ioc->fd = -1;
>      }
>  }
> @@ -173,7 +173,7 @@ static int qio_channel_file_close(QIOChannel *ioc,
>  {
>      QIOChannelFile *fioc = QIO_CHANNEL_FILE(ioc);
>
> -    if (close(fioc->fd) < 0) {
> +    if (qemu_close(fioc->fd) < 0) {
>          error_setg_errno(errp, errno,
>                           "Unable to close file");
>          return -1;
> --
> 2.9.5
>
>



-- 
Marc-André Lureau

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes
  2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
                   ` (3 preceding siblings ...)
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile Ross Lagerwall
@ 2018-01-18 13:40 ` Ross Lagerwall
  2018-01-18 13:47   ` Daniel P. Berrange
  2018-01-18 13:51 ` Daniel P. Berrange
  5 siblings, 1 reply; 16+ messages in thread
From: Ross Lagerwall @ 2018-01-18 13:40 UTC (permalink / raw)
  To: qemu-devel

On 11/01/2017 02:25 PM, Ross Lagerwall wrote:
> Hi,
> 
> Here is a bug fix with the use of QIOChannelFile and 2 bug fixes and an
> improvement to implementation of QIOChannelFile.
> 
> Regards,
> Ross Lagerwall
> 
> Ross Lagerwall (4):
>    migration: Don't leak IO channels
>    io: Fix QIOChannelFile when creating and opening read-write
>    io: Don't call close multiple times in QIOChannelFile
>    io: Add /dev/fdset/ support to QIOChannelFile
> 
>   include/io/channel-file.h    |  2 +-
>   io/channel-file.c            | 11 ++++-------
>   migration/savevm.c           |  2 ++
>   tests/test-io-channel-file.c | 29 +++++++++++++++++++++++++----
>   4 files changed, 32 insertions(+), 12 deletions(-)
> 

Ping for reviews...

v1:
Got feedback from Daniel P. Berrange and Marc-André Lureau.

v2:
Patch 1: Unreviewed
Patch 2: Unreviewed
Patch 3: Reviewed by Marc-André Lureau
Patch 4: Reviewed by Marc-André Lureau

The patch series still applies cleanly on top of master.

Thanks,
-- 
Ross Lagerwall

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
@ 2018-01-18 13:41   ` Ross Lagerwall
  2018-01-18 13:44   ` Daniel P. Berrange
  2018-02-06 11:17   ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 16+ messages in thread
From: Ross Lagerwall @ 2018-01-18 13:41 UTC (permalink / raw)
  To: qemu-devel; +Cc: Juan Quintela, Dr. David Alan Gilbert

On 11/01/2017 02:25 PM, Ross Lagerwall wrote:
> Since qemu_fopen_channel_{in,out}put take references on the underlying
> IO channels, make sure to release our references to them.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> New in v2.
> 
>   migration/savevm.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a88228..87557dd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2259,6 +2259,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
>       }
>       qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
>       f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
> +    object_unref(OBJECT(ioc));
>       ret = qemu_save_device_state(f);
>       qemu_fclose(f);
>       if (ret < 0) {
> @@ -2292,6 +2293,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>       }
>       qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
>       f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
> +    object_unref(OBJECT(ioc));
>   
>       ret = qemu_loadvm_state(f);
>       qemu_fclose(f);
> 

Ping for review...

Thanks,
-- 
Ross Lagerwall

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
  2018-01-18 13:41   ` Ross Lagerwall
@ 2018-01-18 13:44   ` Daniel P. Berrange
  2018-02-06 11:17   ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 16+ messages in thread
From: Daniel P. Berrange @ 2018-01-18 13:44 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel, Dr. David Alan Gilbert, Juan Quintela

On Wed, Nov 01, 2017 at 02:25:23PM +0000, Ross Lagerwall wrote:
> Since qemu_fopen_channel_{in,out}put take references on the underlying
> IO channels, make sure to release our references to them.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> New in v2.
> 
>  migration/savevm.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a88228..87557dd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2259,6 +2259,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
>      }
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
>      f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
> +    object_unref(OBJECT(ioc));
>      ret = qemu_save_device_state(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> @@ -2292,6 +2293,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>      }
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
>      f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
> +    object_unref(OBJECT(ioc));
>  
>      ret = qemu_loadvm_state(f);
>      qemu_fclose(f);
> -- 
> 2.9.5
> 
> 

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 :|

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes
  2018-01-18 13:40 ` [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
@ 2018-01-18 13:47   ` Daniel P. Berrange
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel P. Berrange @ 2018-01-18 13:47 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel

On Thu, Jan 18, 2018 at 01:40:30PM +0000, Ross Lagerwall wrote:
> On 11/01/2017 02:25 PM, Ross Lagerwall wrote:
> > Hi,
> > 
> > Here is a bug fix with the use of QIOChannelFile and 2 bug fixes and an
> > improvement to implementation of QIOChannelFile.
> > 
> > Regards,
> > Ross Lagerwall
> > 
> > Ross Lagerwall (4):
> >    migration: Don't leak IO channels
> >    io: Fix QIOChannelFile when creating and opening read-write
> >    io: Don't call close multiple times in QIOChannelFile
> >    io: Add /dev/fdset/ support to QIOChannelFile
> > 
> >   include/io/channel-file.h    |  2 +-
> >   io/channel-file.c            | 11 ++++-------
> >   migration/savevm.c           |  2 ++
> >   tests/test-io-channel-file.c | 29 +++++++++++++++++++++++++----
> >   4 files changed, 32 insertions(+), 12 deletions(-)
> > 
> 
> Ping for reviews...
> 
> v1:
> Got feedback from Daniel P. Berrange and Marc-André Lureau.
> 
> v2:
> Patch 1: Unreviewed
> Patch 2: Unreviewed
> Patch 3: Reviewed by Marc-André Lureau
> Patch 4: Reviewed by Marc-André Lureau
> 
> The patch series still applies cleanly on top of master.

Sorry my bad for forgetting this. I'll review it now.

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 :|

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/4] io: Fix QIOChannelFile when creating and opening read-write
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 2/4] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
@ 2018-01-18 13:49   ` Daniel P. Berrange
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel P. Berrange @ 2018-01-18 13:49 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel

On Wed, Nov 01, 2017 at 02:25:24PM +0000, Ross Lagerwall wrote:
> The code wrongly passes the mode to open() only if O_WRONLY is set.
> Instead, the mode should be passed when O_CREAT is set (or O_TMPFILE on
> Linux). Fix this by always passing the mode since open() will correctly
> ignore the mode if it is not needed. Add a testcase which exercises this
> bug and also change the existing testcase to check that the mode of the
> created file is correct.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> Changed in v2:
> * Separated from qemu_open() change.
> 
>  include/io/channel-file.h    |  2 +-
>  io/channel-file.c            |  6 +-----
>  tests/test-io-channel-file.c | 29 +++++++++++++++++++++++++----
>  3 files changed, 27 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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 :|

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile Ross Lagerwall
  2017-11-01 18:11   ` Marc-André Lureau
@ 2018-01-18 13:50   ` Daniel P. Berrange
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel P. Berrange @ 2018-01-18 13:50 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel

On Wed, Nov 01, 2017 at 02:25:25PM +0000, Ross Lagerwall wrote:
> If the file descriptor underlying QIOChannelFile is closed in the
> io_close() method, don't close it again in the finalize() method since
> the file descriptor number may have been reused in the meantime.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> New in v2.
> 
>  io/channel-file.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

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 :|

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile Ross Lagerwall
  2017-11-01 18:12   ` Marc-André Lureau
@ 2018-01-18 13:50   ` Daniel P. Berrange
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel P. Berrange @ 2018-01-18 13:50 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel

On Wed, Nov 01, 2017 at 02:25:26PM +0000, Ross Lagerwall wrote:
> Add /dev/fdset/ support to QIOChannelFile by calling qemu_open() instead
> of open() and qemu_close() instead of close(). There is a subtle
> semantic change since qemu_open() automatically sets O_CLOEXEC, but this
> doesn't affect any of the users of the function.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> Changed in v2:
> * Split into separate patch.
> 
>  io/channel-file.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

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 :|

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes
  2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
                   ` (4 preceding siblings ...)
  2018-01-18 13:40 ` [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
@ 2018-01-18 13:51 ` Daniel P. Berrange
  5 siblings, 0 replies; 16+ messages in thread
From: Daniel P. Berrange @ 2018-01-18 13:51 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel

On Wed, Nov 01, 2017 at 02:25:22PM +0000, Ross Lagerwall wrote:
> Hi,
> 
> Here is a bug fix with the use of QIOChannelFile and 2 bug fixes and an
> improvement to implementation of QIOChannelFile.
> 
> Regards,
> Ross Lagerwall
> 
> Ross Lagerwall (4):
>   migration: Don't leak IO channels
>   io: Fix QIOChannelFile when creating and opening read-write
>   io: Don't call close multiple times in QIOChannelFile
>   io: Add /dev/fdset/ support to QIOChannelFile

I've queued the 3 io patches, but will leave the migration one for
the migration maintainer's queue.

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 :|

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels
  2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
  2018-01-18 13:41   ` Ross Lagerwall
  2018-01-18 13:44   ` Daniel P. Berrange
@ 2018-02-06 11:17   ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2018-02-06 11:17 UTC (permalink / raw)
  To: Ross Lagerwall; +Cc: qemu-devel, Juan Quintela

* Ross Lagerwall (ross.lagerwall@citrix.com) wrote:
> Since qemu_fopen_channel_{in,out}put take references on the underlying
> IO channels, make sure to release our references to them.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Queued for migration.

Dave

> ---
> New in v2.
> 
>  migration/savevm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a88228..87557dd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2259,6 +2259,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
>      }
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
>      f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
> +    object_unref(OBJECT(ioc));
>      ret = qemu_save_device_state(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> @@ -2292,6 +2293,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>      }
>      qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
>      f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
> +    object_unref(OBJECT(ioc));
>  
>      ret = qemu_loadvm_state(f);
>      qemu_fclose(f);
> -- 
> 2.9.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-02-06 11:17 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-01 14:25 [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 1/4] migration: Don't leak IO channels Ross Lagerwall
2018-01-18 13:41   ` Ross Lagerwall
2018-01-18 13:44   ` Daniel P. Berrange
2018-02-06 11:17   ` Dr. David Alan Gilbert
2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 2/4] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
2018-01-18 13:49   ` Daniel P. Berrange
2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 3/4] io: Don't call close multiple times in QIOChannelFile Ross Lagerwall
2017-11-01 18:11   ` Marc-André Lureau
2018-01-18 13:50   ` Daniel P. Berrange
2017-11-01 14:25 ` [Qemu-devel] [PATCH v2 4/4] io: Add /dev/fdset/ support to QIOChannelFile Ross Lagerwall
2017-11-01 18:12   ` Marc-André Lureau
2018-01-18 13:50   ` Daniel P. Berrange
2018-01-18 13:40 ` [Qemu-devel] [PATCH v2 0/4] QIOChannelFile bug fixes Ross Lagerwall
2018-01-18 13:47   ` Daniel P. Berrange
2018-01-18 13:51 ` Daniel P. Berrange

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.