1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write

Ross Lagerwall posted 1 patch 6 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171031160902.6874-1-ross.lagerwall@citrix.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
include/io/channel-file.h    |  2 +-
io/channel-file.c            |  6 +-----
tests/test-io-channel-file.c | 27 +++++++++++++++++++++++++++
3 files changed, 29 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
Posted by Ross Lagerwall 6 years, 5 months ago 
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.

While at it, add /dev/fdset/ support to QIOChannelFile by calling
qemu_open() instead open(). 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>
---
 include/io/channel-file.h    |  2 +-
 io/channel-file.c            |  6 +-----
 tests/test-io-channel-file.c | 27 +++++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 6 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..ae8fb62 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 = qemu_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..0c7303d 100644
--- a/tests/test-io-channel-file.c
+++ b/tests/test-io-channel-file.c
@@ -50,6 +50,32 @@ static void test_io_channel_file(void)
     object_unref(OBJECT(dst));
 }
 
+static void test_io_channel_file_rdwr(void)
+{
+    QIOChannel *src, *dst;
+    QIOChannelTest *test;
+
+#define TEST_FILE "tests/test-io-channel-file.txt"
+    unlink(TEST_FILE);
+    src = QIO_CHANNEL(qio_channel_file_new_path(
+                          TEST_FILE,
+                          O_RDWR | O_CREAT | O_TRUNC | O_BINARY, 0600,
+                          &error_abort));
+    dst = QIO_CHANNEL(qio_channel_file_new_path(
+                          TEST_FILE,
+                          O_RDONLY | O_BINARY, 0,
+                          &error_abort));
+
+    test = qio_channel_test_new();
+    qio_channel_test_run_writer(test, src);
+    qio_channel_test_run_reader(test, dst);
+    qio_channel_test_validate(test);
+
+    unlink(TEST_FILE);
+    object_unref(OBJECT(src));
+    object_unref(OBJECT(dst));
+}
+
 
 static void test_io_channel_fd(void)
 {
@@ -114,6 +140,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
Re: [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
Posted by Marc-André Lureau 6 years, 5 months ago 
Hi

(adding the maintainer in cc, scripts/get_maintainer.pl -f  io/channel-file.c)

On Tue, Oct 31, 2017 at 5:09 PM, Ross Lagerwall
<ross.lagerwall@citrix.com> 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.

ok

>
> While at it, add /dev/fdset/ support to QIOChannelFile by calling
> qemu_open() instead open(). 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.
>

yes, I meant to send a similar patch. However, it should probably call
qemu_close() then.


> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  include/io/channel-file.h    |  2 +-
>  io/channel-file.c            |  6 +-----
>  tests/test-io-channel-file.c | 27 +++++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 6 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..ae8fb62 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 = qemu_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..0c7303d 100644
> --- a/tests/test-io-channel-file.c
> +++ b/tests/test-io-channel-file.c
> @@ -50,6 +50,32 @@ static void test_io_channel_file(void)
>      object_unref(OBJECT(dst));
>  }
>
> +static void test_io_channel_file_rdwr(void)
> +{
> +    QIOChannel *src, *dst;
> +    QIOChannelTest *test;
> +
> +#define TEST_FILE "tests/test-io-channel-file.txt"
> +    unlink(TEST_FILE);
> +    src = QIO_CHANNEL(qio_channel_file_new_path(
> +                          TEST_FILE,
> +                          O_RDWR | O_CREAT | O_TRUNC | O_BINARY, 0600,
> +                          &error_abort));
> +    dst = QIO_CHANNEL(qio_channel_file_new_path(
> +                          TEST_FILE,
> +                          O_RDONLY | O_BINARY, 0,
> +                          &error_abort));
> +
> +    test = qio_channel_test_new();
> +    qio_channel_test_run_writer(test, src);
> +    qio_channel_test_run_reader(test, dst);
> +    qio_channel_test_validate(test);
> +
> +    unlink(TEST_FILE);
> +    object_unref(OBJECT(src));
> +    object_unref(OBJECT(dst));
> +}
> +
>
>  static void test_io_channel_fd(void)
>  {
> @@ -114,6 +140,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
>
>

Looks good to me otherwise.


-- 
Marc-André Lureau
Re: [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
Posted by Daniel P. Berrange 6 years, 5 months ago 
On Tue, Oct 31, 2017 at 04:09:02PM +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.
> 
> While at it, add /dev/fdset/ support to QIOChannelFile by calling
> qemu_open() instead open(). 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.

Can you split the use of qemu_open() into a separate patch - its bad pratice
to mix two different functional changes in one patch. Also as Marc-Andre
mentions, we would need qemu_close() here too I think.

> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  include/io/channel-file.h    |  2 +-
>  io/channel-file.c            |  6 +-----
>  tests/test-io-channel-file.c | 27 +++++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 6 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..ae8fb62 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 = qemu_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..0c7303d 100644
> --- a/tests/test-io-channel-file.c
> +++ b/tests/test-io-channel-file.c
> @@ -50,6 +50,32 @@ static void test_io_channel_file(void)
>      object_unref(OBJECT(dst));
>  }
>  
> +static void test_io_channel_file_rdwr(void)
> +{
> +    QIOChannel *src, *dst;
> +    QIOChannelTest *test;
> +
> +#define TEST_FILE "tests/test-io-channel-file.txt"
> +    unlink(TEST_FILE);
> +    src = QIO_CHANNEL(qio_channel_file_new_path(
> +                          TEST_FILE,
> +                          O_RDWR | O_CREAT | O_TRUNC | O_BINARY, 0600,
> +                          &error_abort));
> +    dst = QIO_CHANNEL(qio_channel_file_new_path(
> +                          TEST_FILE,
> +                          O_RDONLY | O_BINARY, 0,
> +                          &error_abort));
> +
> +    test = qio_channel_test_new();
> +    qio_channel_test_run_writer(test, src);
> +    qio_channel_test_run_reader(test, dst);
> +    qio_channel_test_validate(test);

Since we're specifically fixing a problem with mode, it would be good to
check the mode of the created file to show that its applied - ideally the
test would fail with previous code in this way.

> +
> +    unlink(TEST_FILE);
> +    object_unref(OBJECT(src));
> +    object_unref(OBJECT(dst));
> +}
> +

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-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
Posted by Ross Lagerwall 6 years, 5 months ago 
On 11/01/2017 10:04 AM, Daniel P. Berrange wrote:
> On Tue, Oct 31, 2017 at 04:09:02PM +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.
>>
>> While at it, add /dev/fdset/ support to QIOChannelFile by calling
>> qemu_open() instead open(). 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.
> 
> Can you split the use of qemu_open() into a separate patch - its bad pratice
> to mix two different functional changes in one patch. Also as Marc-Andre
> mentions, we would need qemu_close() here too I think.

OK, sure, I will split into two patches and call qemu_close().

> 
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   include/io/channel-file.h    |  2 +-
>>   io/channel-file.c            |  6 +-----
>>   tests/test-io-channel-file.c | 27 +++++++++++++++++++++++++++
>>   3 files changed, 29 insertions(+), 6 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..ae8fb62 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 = qemu_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..0c7303d 100644
>> --- a/tests/test-io-channel-file.c
>> +++ b/tests/test-io-channel-file.c
>> @@ -50,6 +50,32 @@ static void test_io_channel_file(void)
>>       object_unref(OBJECT(dst));
>>   }
>>   
>> +static void test_io_channel_file_rdwr(void)
>> +{
>> +    QIOChannel *src, *dst;
>> +    QIOChannelTest *test;
>> +
>> +#define TEST_FILE "tests/test-io-channel-file.txt"
>> +    unlink(TEST_FILE);
>> +    src = QIO_CHANNEL(qio_channel_file_new_path(
>> +                          TEST_FILE,
>> +                          O_RDWR | O_CREAT | O_TRUNC | O_BINARY, 0600,
>> +                          &error_abort));
>> +    dst = QIO_CHANNEL(qio_channel_file_new_path(
>> +                          TEST_FILE,
>> +                          O_RDONLY | O_BINARY, 0,
>> +                          &error_abort));
>> +
>> +    test = qio_channel_test_new();
>> +    qio_channel_test_run_writer(test, src);
>> +    qio_channel_test_run_reader(test, dst);
>> +    qio_channel_test_validate(test);
> 
> Since we're specifically fixing a problem with mode, it would be good to
> check the mode of the created file to show that its applied - ideally the
> test would fail with previous code in this way.

At least with glibc, the test does fail with the previous code because 
it aborts if O_CREAT is present in flags without the mode. But I can add 
a check anyway.

Regards,
-- 
Ross Lagerwall

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.