* [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
@ 2017-10-31 16:09 Ross Lagerwall
2017-10-31 16:49 ` Marc-André Lureau
2017-11-01 10:04 ` Daniel P. Berrange
0 siblings, 2 replies; 4+ messages in thread
From: Ross Lagerwall @ 2017-10-31 16:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrange, Ross Lagerwall
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
2017-10-31 16:09 [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
@ 2017-10-31 16:49 ` Marc-André Lureau
2017-11-01 10:04 ` Daniel P. Berrange
1 sibling, 0 replies; 4+ messages in thread
From: Marc-André Lureau @ 2017-10-31 16:49 UTC (permalink / raw)
To: Ross Lagerwall, Daniel P. Berrange; +Cc: QEMU
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
2017-10-31 16:09 [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
2017-10-31 16:49 ` Marc-André Lureau
@ 2017-11-01 10:04 ` Daniel P. Berrange
2017-11-01 10:10 ` Ross Lagerwall
1 sibling, 1 reply; 4+ messages in thread
From: Daniel P. Berrange @ 2017-11-01 10:04 UTC (permalink / raw)
To: Ross Lagerwall; +Cc: qemu-devel
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 :|
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write
2017-11-01 10:04 ` Daniel P. Berrange
@ 2017-11-01 10:10 ` Ross Lagerwall
0 siblings, 0 replies; 4+ messages in thread
From: Ross Lagerwall @ 2017-11-01 10:10 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-11-01 10:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-31 16:09 [Qemu-devel] [PATCH] io: Fix QIOChannelFile when creating and opening read-write Ross Lagerwall
2017-10-31 16:49 ` Marc-André Lureau
2017-11-01 10:04 ` Daniel P. Berrange
2017-11-01 10:10 ` Ross Lagerwall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).