All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PULL 0/6] Block patches for rc2
@ 2017-03-27 15:52 Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 1/6] qemu-img: show help for invalid global options Max Reitz
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell

The following changes since commit ea2afcf5b6727a577cf561fd8fe0d8c397ecc927:

  Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' into staging (2017-03-24 14:14:18 +0000)

are available in the git repository at:

  git://github.com/XanClic/qemu.git tags/pull-block-2017-03-27

for you to fetch changes up to 700f9ce0f9f49675818ac95c8953cbc65aa1d112:

  block/file-posix.c: Fix unused variable warning on OpenBSD (2017-03-27 17:28:34 +0200)

----------------------------------------------------------------
Block patches for 2.9-rc2.

----------------------------------------------------------------
Hi Peter,

This pull request contains your build fix for OpenBSD on top so you can
just drop it if you have merged it yourself already.


Thanks,

Max

----------------------------------------------------------------
Kevin Wolf (1):
      file-posix: Make bdrv_flush() failure permanent without O_DIRECT

Paolo Bonzini (1):
      nbd-client: fix handling of hungup connections

Peter Maydell (1):
      block/file-posix.c: Fix unused variable warning on OpenBSD

Stefan Hajnoczi (3):
      qemu-img: show help for invalid global options
      qemu-img: fix switch indentation in img_amend()
      qemu-img: print short help on getopt failure

 block/file-posix.c |  50 ++++++++++----
 block/nbd-client.c |  12 ++--
 nbd/client.c       |   2 +-
 qemu-img.c         | 196 +++++++++++++++++++++++++++++++++++++----------------
 4 files changed, 180 insertions(+), 80 deletions(-)

-- 
2.12.1

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

* [Qemu-devel] [PULL 1/6] qemu-img: show help for invalid global options
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
@ 2017-03-27 15:52 ` Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 2/6] qemu-img: fix switch indentation in img_amend() Max Reitz
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell, Stefan Hajnoczi

From: Stefan Hajnoczi <stefanha@redhat.com>

The qemu-img sub-command executes regardless of invalid global options:

  $ qemu-img --foo info test.img
  qemu-img: unrecognized option '--foo'
  image: test.img
  ...

The unrecognized option warning may be missed by the user.  This can
hide incorrect command-lines in scripts and confuse users.

This patch prints the help information and terminates instead of
executing the sub-command.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20170317104541.28979-2-stefanha@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-img.c b/qemu-img.c
index 98b836b030..ce293a4710 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4339,6 +4339,7 @@ int main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "+hVT:", long_options, NULL)) != -1) {
         switch (c) {
         case 'h':
+        case '?':
             help();
             return 0;
         case 'V':
-- 
2.12.1

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

* [Qemu-devel] [PULL 2/6] qemu-img: fix switch indentation in img_amend()
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 1/6] qemu-img: show help for invalid global options Max Reitz
@ 2017-03-27 15:52 ` Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 3/6] qemu-img: print short help on getopt failure Max Reitz
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell, Stefan Hajnoczi

From: Stefan Hajnoczi <stefanha@redhat.com>

QEMU coding style indents 'case' to the same level as the 'switch'
statement:

  switch (foo) {
  case 1:

Fix this coding style violation so checkpatch.pl doesn't complain about
the next patch.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20170317104541.28979-3-stefanha@redhat.com
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 82 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ce293a4710..c7ffabb268 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3500,47 +3500,47 @@ static int img_amend(int argc, char **argv)
         }
 
         switch (c) {
-            case 'h':
-            case '?':
-                help();
-                break;
-            case 'o':
-                if (!is_valid_option_list(optarg)) {
-                    error_report("Invalid option list: %s", optarg);
-                    ret = -1;
-                    goto out_no_progress;
-                }
-                if (!options) {
-                    options = g_strdup(optarg);
-                } else {
-                    char *old_options = options;
-                    options = g_strdup_printf("%s,%s", options, optarg);
-                    g_free(old_options);
-                }
-                break;
-            case 'f':
-                fmt = optarg;
-                break;
-            case 't':
-                cache = optarg;
-                break;
-            case 'p':
-                progress = true;
-                break;
-            case 'q':
-                quiet = true;
-                break;
-            case OPTION_OBJECT:
-                opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                               optarg, true);
-                if (!opts) {
-                    ret = -1;
-                    goto out_no_progress;
-                }
-                break;
-            case OPTION_IMAGE_OPTS:
-                image_opts = true;
-                break;
+        case 'h':
+        case '?':
+            help();
+            break;
+        case 'o':
+            if (!is_valid_option_list(optarg)) {
+                error_report("Invalid option list: %s", optarg);
+                ret = -1;
+                goto out_no_progress;
+            }
+            if (!options) {
+                options = g_strdup(optarg);
+            } else {
+                char *old_options = options;
+                options = g_strdup_printf("%s,%s", options, optarg);
+                g_free(old_options);
+            }
+            break;
+        case 'f':
+            fmt = optarg;
+            break;
+        case 't':
+            cache = optarg;
+            break;
+        case 'p':
+            progress = true;
+            break;
+        case 'q':
+            quiet = true;
+            break;
+        case OPTION_OBJECT:
+            opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                           optarg, true);
+            if (!opts) {
+                ret = -1;
+                goto out_no_progress;
+            }
+            break;
+        case OPTION_IMAGE_OPTS:
+            image_opts = true;
+            break;
         }
     }
 
-- 
2.12.1

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

* [Qemu-devel] [PULL 3/6] qemu-img: print short help on getopt failure
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 1/6] qemu-img: show help for invalid global options Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 2/6] qemu-img: fix switch indentation in img_amend() Max Reitz
@ 2017-03-27 15:52 ` Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 4/6] nbd-client: fix handling of hungup connections Max Reitz
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell, Stefan Hajnoczi

From: Stefan Hajnoczi <stefanha@redhat.com>

Printing the full help output obscures the error message for an invalid
command-line option or missing argument.

Before this patch:

  $ ./qemu-img --foo
  ...pages of output...

After this patch:

  $ ./qemu-img --foo
  qemu-img: unrecognized option '--foo'
  Try 'qemu-img --help' for more information

This patch adds the getopt ':' character so that it can distinguish
between missing arguments and unrecognized options.  This helps provide
more detailed error messages.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20170317104541.28979-4-stefanha@redhat.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 97 insertions(+), 20 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c7ffabb268..b220cf71d7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -88,6 +88,16 @@ static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)
     exit(EXIT_FAILURE);
 }
 
+static void QEMU_NORETURN missing_argument(const char *option)
+{
+    error_exit("missing argument for option '%s'", option);
+}
+
+static void QEMU_NORETURN unrecognized_option(const char *option)
+{
+    error_exit("unrecognized option '%s'", option);
+}
+
 /* Please keep in synch with qemu-img.texi */
 static void QEMU_NORETURN help(void)
 {
@@ -406,13 +416,18 @@ static int img_create(int argc, char **argv)
             {"object", required_argument, 0, OPTION_OBJECT},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "F:b:f:he6o:q",
+        c = getopt_long(argc, argv, ":F:b:f:he6o:q",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -651,13 +666,18 @@ static int img_check(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:r:T:q",
+        c = getopt_long(argc, argv, ":hf:r:T:q",
                         long_options, &option_index);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -855,13 +875,18 @@ static int img_commit(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:ht:b:dpq",
+        c = getopt_long(argc, argv, ":f:ht:b:dpq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -1190,13 +1215,18 @@ static int img_compare(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:T:pqs",
+        c = getopt_long(argc, argv, ":hf:F:T:pqs",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch (c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -1926,13 +1956,18 @@ static int img_convert(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qnm:W",
+        c = getopt_long(argc, argv, ":hf:O:B:ce6o:s:l:S:pt:T:qnm:W",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -2502,13 +2537,18 @@ static int img_info(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, ":f:h",
                         long_options, &option_index);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -2713,13 +2753,18 @@ static int img_map(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, ":f:h",
                         long_options, &option_index);
         if (c == -1) {
             break;
         }
         switch (c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -2835,13 +2880,18 @@ static int img_snapshot(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "la:c:d:hq",
+        c = getopt_long(argc, argv, ":la:c:d:hq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             return 0;
@@ -2988,13 +3038,18 @@ static int img_rebase(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:b:upt:T:q",
+        c = getopt_long(argc, argv, ":hf:F:b:upt:T:q",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             return 0;
@@ -3355,13 +3410,18 @@ static int img_resize(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hq",
+        c = getopt_long(argc, argv, ":f:hq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
         switch(c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -3493,15 +3553,20 @@ static int img_amend(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "ho:f:t:pq",
+        c = getopt_long(argc, argv, ":ho:f:t:pq",
                         long_options, NULL);
         if (c == -1) {
             break;
         }
 
         switch (c) {
-        case 'h':
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
+        case 'h':
             help();
             break;
         case 'o':
@@ -3759,14 +3824,19 @@ static int img_bench(int argc, char **argv)
             {"no-drain", no_argument, 0, OPTION_NO_DRAIN},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hc:d:f:no:qs:S:t:w", long_options, NULL);
+        c = getopt_long(argc, argv, ":hc:d:f:no:qs:S:t:w", long_options, NULL);
         if (c == -1) {
             break;
         }
 
         switch (c) {
-        case 'h':
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
+        case 'h':
             help();
             break;
         case 'c':
@@ -4093,7 +4163,7 @@ static int img_dd(int argc, char **argv)
         { 0, 0, 0, 0 }
     };
 
-    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
+    while ((c = getopt_long(argc, argv, ":hf:O:", long_options, NULL))) {
         if (c == EOF) {
             break;
         }
@@ -4104,10 +4174,12 @@ static int img_dd(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
         case '?':
-            error_report("Try 'qemu-img --help' for more information.");
-            ret = -1;
-            goto out;
+            unrecognized_option(argv[optind - 1]);
+            break;
         case 'h':
             help();
             break;
@@ -4336,10 +4408,15 @@ int main(int argc, char **argv)
     qemu_add_opts(&qemu_source_opts);
     qemu_add_opts(&qemu_trace_opts);
 
-    while ((c = getopt_long(argc, argv, "+hVT:", long_options, NULL)) != -1) {
+    while ((c = getopt_long(argc, argv, "+:hVT:", long_options, NULL)) != -1) {
         switch (c) {
-        case 'h':
+        case ':':
+            missing_argument(argv[optind - 1]);
+            return 0;
         case '?':
+            unrecognized_option(argv[optind - 1]);
+            return 0;
+        case 'h':
             help();
             return 0;
         case 'V':
-- 
2.12.1

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

* [Qemu-devel] [PULL 4/6] nbd-client: fix handling of hungup connections
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
                   ` (2 preceding siblings ...)
  2017-03-27 15:52 ` [Qemu-devel] [PULL 3/6] qemu-img: print short help on getopt failure Max Reitz
@ 2017-03-27 15:52 ` Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 5/6] file-posix: Make bdrv_flush() failure permanent without O_DIRECT Max Reitz
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell, Paolo Bonzini

From: Paolo Bonzini <pbonzini@redhat.com>

After the switch to reading replies in a coroutine, nothing is
reentering pending receive coroutines if the connection hangs.
Move nbd_recv_coroutines_enter_all to the reply read coroutine,
which is the place where hangups are detected.  nbd_teardown_connection
can simply wait for the reply read coroutine to detect the hangup
and clean up after itself.

This wouldn't be enough though because nbd_receive_reply returns 0
(rather than -EPIPE or similar) when reading from a hung connection.
Fix the return value check in nbd_read_reply_entry.

This fixes qemu-iotests 083.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 20170314111157.14464-1-pbonzini@redhat.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd-client.c | 12 ++++++------
 nbd/client.c       |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 0dc12c2d67..1e2952fdae 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -33,17 +33,15 @@
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ ((uint64_t)(intptr_t)bs))
 
-static void nbd_recv_coroutines_enter_all(BlockDriverState *bs)
+static void nbd_recv_coroutines_enter_all(NBDClientSession *s)
 {
-    NBDClientSession *s = nbd_get_client_session(bs);
     int i;
 
     for (i = 0; i < MAX_NBD_REQUESTS; i++) {
         if (s->recv_coroutine[i]) {
-            qemu_coroutine_enter(s->recv_coroutine[i]);
+            aio_co_wake(s->recv_coroutine[i]);
         }
     }
-    BDRV_POLL_WHILE(bs, s->read_reply_co);
 }
 
 static void nbd_teardown_connection(BlockDriverState *bs)
@@ -58,7 +56,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
     qio_channel_shutdown(client->ioc,
                          QIO_CHANNEL_SHUTDOWN_BOTH,
                          NULL);
-    nbd_recv_coroutines_enter_all(bs);
+    BDRV_POLL_WHILE(bs, client->read_reply_co);
 
     nbd_client_detach_aio_context(bs);
     object_unref(OBJECT(client->sioc));
@@ -76,7 +74,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
     for (;;) {
         assert(s->reply.handle == 0);
         ret = nbd_receive_reply(s->ioc, &s->reply);
-        if (ret < 0) {
+        if (ret <= 0) {
             break;
         }
 
@@ -103,6 +101,8 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
         aio_co_wake(s->recv_coroutine[i]);
         qemu_coroutine_yield();
     }
+
+    nbd_recv_coroutines_enter_all(s);
     s->read_reply_co = NULL;
 }
 
diff --git a/nbd/client.c b/nbd/client.c
index 3dc2564cd0..a58fb02cb4 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -812,6 +812,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
         LOG("invalid magic (got 0x%" PRIx32 ")", magic);
         return -EINVAL;
     }
-    return 0;
+    return sizeof(buf);
 }
 
-- 
2.12.1

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

* [Qemu-devel] [PULL 5/6] file-posix: Make bdrv_flush() failure permanent without O_DIRECT
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
                   ` (3 preceding siblings ...)
  2017-03-27 15:52 ` [Qemu-devel] [PULL 4/6] nbd-client: fix handling of hungup connections Max Reitz
@ 2017-03-27 15:52 ` Max Reitz
  2017-03-27 15:52 ` [Qemu-devel] [PULL 6/6] block/file-posix.c: Fix unused variable warning on OpenBSD Max Reitz
  2017-03-27 16:34 ` [Qemu-devel] [PULL 0/6] Block patches for rc2 Peter Maydell
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell, Kevin Wolf

From: Kevin Wolf <kwolf@redhat.com>

Success for bdrv_flush() means that all previously written data is safe
on disk. For fdatasync(), the best semantics we can hope for on Linux
(without O_DIRECT) is that all data that was written since the last call
was successfully written back. Therefore, and because we can't redo all
writes after a flush failure, we have to give up after a single
fdatasync() failure. After this failure, we would never be able to make
the promise that a successful bdrv_flush() makes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20170322210005.16533-1-kwolf@redhat.com
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 53febd3767..beb7a4f728 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -144,6 +144,7 @@ typedef struct BDRVRawState {
     bool has_write_zeroes:1;
     bool discard_zeroes:1;
     bool use_linux_aio:1;
+    bool page_cache_inconsistent:1;
     bool has_fallocate;
     bool needs_alignment;
 } BDRVRawState;
@@ -824,10 +825,31 @@ static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
 
 static ssize_t handle_aiocb_flush(RawPosixAIOData *aiocb)
 {
+    BDRVRawState *s = aiocb->bs->opaque;
     int ret;
 
+    if (s->page_cache_inconsistent) {
+        return -EIO;
+    }
+
     ret = qemu_fdatasync(aiocb->aio_fildes);
     if (ret == -1) {
+        /* There is no clear definition of the semantics of a failing fsync(),
+         * so we may have to assume the worst. The sad truth is that this
+         * assumption is correct for Linux. Some pages are now probably marked
+         * clean in the page cache even though they are inconsistent with the
+         * on-disk contents. The next fdatasync() call would succeed, but no
+         * further writeback attempt will be made. We can't get back to a state
+         * in which we know what is on disk (we would have to rewrite
+         * everything that was touched since the last fdatasync() at least), so
+         * make bdrv_flush() fail permanently. Given that the behaviour isn't
+         * really defined, I have little hope that other OSes are doing better.
+         *
+         * Obviously, this doesn't affect O_DIRECT, which bypasses the page
+         * cache. */
+        if ((s->open_flags & O_DIRECT) == 0) {
+            s->page_cache_inconsistent = true;
+        }
         return -errno;
     }
     return 0;
-- 
2.12.1

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

* [Qemu-devel] [PULL 6/6] block/file-posix.c: Fix unused variable warning on OpenBSD
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
                   ` (4 preceding siblings ...)
  2017-03-27 15:52 ` [Qemu-devel] [PULL 5/6] file-posix: Make bdrv_flush() failure permanent without O_DIRECT Max Reitz
@ 2017-03-27 15:52 ` Max Reitz
  2017-03-27 16:34 ` [Qemu-devel] [PULL 0/6] Block patches for rc2 Peter Maydell
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2017-03-27 15:52 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, Peter Maydell

From: Peter Maydell <peter.maydell@linaro.org>

On OpenBSD none of the ioctls probe_logical_blocksize() tries
exist, so the variable sector_size is unused. Refactor the
code to avoid this (and reduce the duplicated code).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 1490279788-12995-1-git-send-email-peter.maydell@linaro.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index beb7a4f728..0841a08785 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -220,28 +220,28 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)
 {
     unsigned int sector_size;
     bool success = false;
+    int i;
 
     errno = ENOTSUP;
-
-    /* Try a few ioctls to get the right size */
+    static const unsigned long ioctl_list[] = {
 #ifdef BLKSSZGET
-    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
-        *sector_size_p = sector_size;
-        success = true;
-    }
+        BLKSSZGET,
 #endif
 #ifdef DKIOCGETBLOCKSIZE
-    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
-        *sector_size_p = sector_size;
-        success = true;
-    }
+        DKIOCGETBLOCKSIZE,
 #endif
 #ifdef DIOCGSECTORSIZE
-    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
-        *sector_size_p = sector_size;
-        success = true;
-    }
+        DIOCGSECTORSIZE,
 #endif
+    };
+
+    /* Try a few ioctls to get the right size */
+    for (i = 0; i < (int)ARRAY_SIZE(ioctl_list); i++) {
+        if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {
+            *sector_size_p = sector_size;
+            success = true;
+        }
+    }
 
     return success ? 0 : -errno;
 }
-- 
2.12.1

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

* Re: [Qemu-devel] [PULL 0/6] Block patches for rc2
  2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
                   ` (5 preceding siblings ...)
  2017-03-27 15:52 ` [Qemu-devel] [PULL 6/6] block/file-posix.c: Fix unused variable warning on OpenBSD Max Reitz
@ 2017-03-27 16:34 ` Peter Maydell
  6 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2017-03-27 16:34 UTC (permalink / raw)
  To: Max Reitz; +Cc: Qemu-block, QEMU Developers

On 27 March 2017 at 16:52, Max Reitz <mreitz@redhat.com> wrote:
> The following changes since commit ea2afcf5b6727a577cf561fd8fe0d8c397ecc927:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' into staging (2017-03-24 14:14:18 +0000)
>
> are available in the git repository at:
>
>   git://github.com/XanClic/qemu.git tags/pull-block-2017-03-27
>
> for you to fetch changes up to 700f9ce0f9f49675818ac95c8953cbc65aa1d112:
>
>   block/file-posix.c: Fix unused variable warning on OpenBSD (2017-03-27 17:28:34 +0200)
>
> ----------------------------------------------------------------
> Block patches for 2.9-rc2.
>
> ----------------------------------------------------------------
> Hi Peter,
>
> This pull request contains your build fix for OpenBSD on top so you can
> just drop it if you have merged it yourself already.

I can't take partial pullrequests like that, because the thing you
have signed is the tag at the top of the branch, not the commit
before it. However, git merge will happily handle the branch
containing a commit that's been merged already via some other
route, so that's no problem.

(In this case I hadn't actually merged the buildfix.)

Applied to master.

thanks
-- PMM

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

end of thread, other threads:[~2017-03-27 16:34 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-27 15:52 [Qemu-devel] [PULL 0/6] Block patches for rc2 Max Reitz
2017-03-27 15:52 ` [Qemu-devel] [PULL 1/6] qemu-img: show help for invalid global options Max Reitz
2017-03-27 15:52 ` [Qemu-devel] [PULL 2/6] qemu-img: fix switch indentation in img_amend() Max Reitz
2017-03-27 15:52 ` [Qemu-devel] [PULL 3/6] qemu-img: print short help on getopt failure Max Reitz
2017-03-27 15:52 ` [Qemu-devel] [PULL 4/6] nbd-client: fix handling of hungup connections Max Reitz
2017-03-27 15:52 ` [Qemu-devel] [PULL 5/6] file-posix: Make bdrv_flush() failure permanent without O_DIRECT Max Reitz
2017-03-27 15:52 ` [Qemu-devel] [PULL 6/6] block/file-posix.c: Fix unused variable warning on OpenBSD Max Reitz
2017-03-27 16:34 ` [Qemu-devel] [PULL 0/6] Block patches for rc2 Peter Maydell

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.