| Message ID | 1435664924-9377-2-git-send-email-rjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
"Richard W.M. Jones" <rjones@redhat.com> writes: > Currently if qemu is connected to a curl source (eg. web server), and > the web server fails / times out / dies, you always see a bogus EIO > "Input/output error". > > For example, choose a large file located on any local webserver which > you control: > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > Once it starts copying the file, stop the webserver and you will see > qemu-img fail with: > > qemu-img: error while reading sector 61440: Input/output error > > This patch does two things: Firstly print the actual error from curl > so it doesn't get lost. Secondly, change EIO to EPROTO. EPROTO is a > POSIX.1 compatible errno which more accurately reflects that there was > a protocol error, rather than some kind of hardware failure. > > After this patch is applied, the error changes to: > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > qemu-img: curl: transfer closed with 469989 bytes remaining to read > qemu-img: error while reading sector 16384: Protocol error > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > block/curl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/block/curl.c b/block/curl.c > index 3a2b63e..2fd7c06 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -22,6 +22,7 @@ > * THE SOFTWARE. > */ > #include "qemu-common.h" > +#include "qemu/error-report.h" > #include "block/block_int.h" > #include "qapi/qmp/qbool.h" > #include "qapi/qmp/qstring.h" > @@ -298,6 +299,12 @@ static void curl_multi_check_completion(BDRVCURLState *s) > /* ACBs for successful messages get completed in curl_read_cb */ > if (msg->data.result != CURLE_OK) { > int i; > + > + /* Don't lose the original error message from curl, since > + * it contains extra data. > + */ > + error_report("curl: %s", state->errmsg); > + > for (i = 0; i < CURL_NUM_ACB; i++) { > CURLAIOCB *acb = state->acb[i]; > Printing an error message, then returning an error code is problematic. It works when the caller is going to print its own error message to the same destination. Callee produces a specific error message devoid of context, caller produces an unspecific one with hopefully more context. Better than just one of them. Worse than a single specific error with context, but that can't be done with just a "return errno code" interface. It's kind of wrong when the caller reports its own error somewhere else, e.g. to a monitor. Still, when barfing extra info to stderr is the best we can do, it's better than nothing. It's more wrong when the caller handles the error quietly. I guess that's never the case here, but I can't be sure without a lot more sleuthing. Perhaps Kevin or Stefan can judge this immediately. > @@ -305,7 +312,7 @@ static void curl_multi_check_completion(BDRVCURLState *s) > continue; > } > > - acb->common.cb(acb->common.opaque, -EIO); > + acb->common.cb(acb->common.opaque, -EPROTO); > qemu_aio_unref(acb); > state->acb[i] = NULL; > } To understand impact exactly, we'd need to figure out where the changed error code gets consumed. However, I don't expect consumers to check the actual error code. A quick grep for comparisons with EIO or -EIO finds nothing related to block I/O, except for nbd_trip() checking the value of nbd_co_receive_request(), and that's unrelated.
Am 03.07.2015 um 14:35 hat Markus Armbruster geschrieben: > "Richard W.M. Jones" <rjones@redhat.com> writes: > > > Currently if qemu is connected to a curl source (eg. web server), and > > the web server fails / times out / dies, you always see a bogus EIO > > "Input/output error". > > > > For example, choose a large file located on any local webserver which > > you control: > > > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > > > Once it starts copying the file, stop the webserver and you will see > > qemu-img fail with: > > > > qemu-img: error while reading sector 61440: Input/output error > > > > This patch does two things: Firstly print the actual error from curl > > so it doesn't get lost. Secondly, change EIO to EPROTO. EPROTO is a > > POSIX.1 compatible errno which more accurately reflects that there was > > a protocol error, rather than some kind of hardware failure. > > > > After this patch is applied, the error changes to: > > > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > qemu-img: curl: transfer closed with 469989 bytes remaining to read > > qemu-img: error while reading sector 16384: Protocol error > > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > block/curl.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/block/curl.c b/block/curl.c > > index 3a2b63e..2fd7c06 100644 > > --- a/block/curl.c > > +++ b/block/curl.c > > @@ -22,6 +22,7 @@ > > * THE SOFTWARE. > > */ > > #include "qemu-common.h" > > +#include "qemu/error-report.h" > > #include "block/block_int.h" > > #include "qapi/qmp/qbool.h" > > #include "qapi/qmp/qstring.h" > > @@ -298,6 +299,12 @@ static void curl_multi_check_completion(BDRVCURLState *s) > > /* ACBs for successful messages get completed in curl_read_cb */ > > if (msg->data.result != CURLE_OK) { > > int i; > > + > > + /* Don't lose the original error message from curl, since > > + * it contains extra data. > > + */ > > + error_report("curl: %s", state->errmsg); > > + > > for (i = 0; i < CURL_NUM_ACB; i++) { > > CURLAIOCB *acb = state->acb[i]; > > > > Printing an error message, then returning an error code is problematic. > > It works when the caller is going to print its own error message to the > same destination. Callee produces a specific error message devoid of > context, caller produces an unspecific one with hopefully more context. > Better than just one of them. Worse than a single specific error with > context, but that can't be done with just a "return errno code" > interface. > > It's kind of wrong when the caller reports its own error somewhere else, > e.g. to a monitor. Still, when barfing extra info to stderr is the best > we can do, it's better than nothing. > > It's more wrong when the caller handles the error quietly. I guess > that's never the case here, but I can't be sure without a lot more > sleuthing. Perhaps Kevin or Stefan can judge this immediately. I'm not worried too much about requests made by the monitor or during startup. I don't like the error_report() there, but having a more specific error message on stderr is better than having nothing. The case that bothers me more is guest requests. Depending on the werror/rerror settings, this may allow the guest to flood the log file with curl error messages. > > @@ -305,7 +312,7 @@ static void curl_multi_check_completion(BDRVCURLState *s) > > continue; > > } > > > > - acb->common.cb(acb->common.opaque, -EIO); > > + acb->common.cb(acb->common.opaque, -EPROTO); > > qemu_aio_unref(acb); > > state->acb[i] = NULL; > > } > > To understand impact exactly, we'd need to figure out where the changed > error code gets consumed. However, I don't expect consumers to check > the actual error code. A quick grep for comparisons with EIO or -EIO > finds nothing related to block I/O, except for nbd_trip() checking the > value of nbd_co_receive_request(), and that's unrelated. Yes, I wouldn't expect any problems caused by this change. Kevin
On Wed, Jul 08, 2015 at 12:23:37PM +0200, Kevin Wolf wrote: > Am 03.07.2015 um 14:35 hat Markus Armbruster geschrieben: > > "Richard W.M. Jones" <rjones@redhat.com> writes: > > > > > Currently if qemu is connected to a curl source (eg. web server), and > > > the web server fails / times out / dies, you always see a bogus EIO > > > "Input/output error". > > > > > > For example, choose a large file located on any local webserver which > > > you control: > > > > > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > > > > > Once it starts copying the file, stop the webserver and you will see > > > qemu-img fail with: > > > > > > qemu-img: error while reading sector 61440: Input/output error > > > > > > This patch does two things: Firstly print the actual error from curl > > > so it doesn't get lost. Secondly, change EIO to EPROTO. EPROTO is a > > > POSIX.1 compatible errno which more accurately reflects that there was > > > a protocol error, rather than some kind of hardware failure. > > > > > > After this patch is applied, the error changes to: > > > > > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > > qemu-img: curl: transfer closed with 469989 bytes remaining to read > > > qemu-img: error while reading sector 16384: Protocol error > > > > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > block/curl.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/block/curl.c b/block/curl.c > > > index 3a2b63e..2fd7c06 100644 > > > --- a/block/curl.c > > > +++ b/block/curl.c > > > @@ -22,6 +22,7 @@ > > > * THE SOFTWARE. > > > */ > > > #include "qemu-common.h" > > > +#include "qemu/error-report.h" > > > #include "block/block_int.h" > > > #include "qapi/qmp/qbool.h" > > > #include "qapi/qmp/qstring.h" > > > @@ -298,6 +299,12 @@ static void curl_multi_check_completion(BDRVCURLState *s) > > > /* ACBs for successful messages get completed in curl_read_cb */ > > > if (msg->data.result != CURLE_OK) { > > > int i; > > > + > > > + /* Don't lose the original error message from curl, since > > > + * it contains extra data. > > > + */ > > > + error_report("curl: %s", state->errmsg); > > > + > > > for (i = 0; i < CURL_NUM_ACB; i++) { > > > CURLAIOCB *acb = state->acb[i]; > > > > > > > Printing an error message, then returning an error code is problematic. > > > > It works when the caller is going to print its own error message to the > > same destination. Callee produces a specific error message devoid of > > context, caller produces an unspecific one with hopefully more context. > > Better than just one of them. Worse than a single specific error with > > context, but that can't be done with just a "return errno code" > > interface. > > > > It's kind of wrong when the caller reports its own error somewhere else, > > e.g. to a monitor. Still, when barfing extra info to stderr is the best > > we can do, it's better than nothing. > > > > It's more wrong when the caller handles the error quietly. I guess > > that's never the case here, but I can't be sure without a lot more > > sleuthing. Perhaps Kevin or Stefan can judge this immediately. > > I'm not worried too much about requests made by the monitor or during > startup. I don't like the error_report() there, but having a more > specific error message on stderr is better than having nothing. > > The case that bothers me more is guest requests. Depending on the > werror/rerror settings, this may allow the guest to flood the log file > with curl error messages. Can you expand a bit on how they would do this? I can see how the remote web server can cause a curl error (itself possibly a concern), but not how the guest can do it. Rich.
Am 08.07.2015 um 13:36 hat Richard W.M. Jones geschrieben: > On Wed, Jul 08, 2015 at 12:23:37PM +0200, Kevin Wolf wrote: > > Am 03.07.2015 um 14:35 hat Markus Armbruster geschrieben: > > > "Richard W.M. Jones" <rjones@redhat.com> writes: > > > > > > > Currently if qemu is connected to a curl source (eg. web server), and > > > > the web server fails / times out / dies, you always see a bogus EIO > > > > "Input/output error". > > > > > > > > For example, choose a large file located on any local webserver which > > > > you control: > > > > > > > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > > > > > > > Once it starts copying the file, stop the webserver and you will see > > > > qemu-img fail with: > > > > > > > > qemu-img: error while reading sector 61440: Input/output error > > > > > > > > This patch does two things: Firstly print the actual error from curl > > > > so it doesn't get lost. Secondly, change EIO to EPROTO. EPROTO is a > > > > POSIX.1 compatible errno which more accurately reflects that there was > > > > a protocol error, rather than some kind of hardware failure. > > > > > > > > After this patch is applied, the error changes to: > > > > > > > > $ qemu-img convert -p http://example.com/large.iso /tmp/test > > > > qemu-img: curl: transfer closed with 469989 bytes remaining to read > > > > qemu-img: error while reading sector 16384: Protocol error > > > > > > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > --- > > > > block/curl.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/block/curl.c b/block/curl.c > > > > index 3a2b63e..2fd7c06 100644 > > > > --- a/block/curl.c > > > > +++ b/block/curl.c > > > > @@ -22,6 +22,7 @@ > > > > * THE SOFTWARE. > > > > */ > > > > #include "qemu-common.h" > > > > +#include "qemu/error-report.h" > > > > #include "block/block_int.h" > > > > #include "qapi/qmp/qbool.h" > > > > #include "qapi/qmp/qstring.h" > > > > @@ -298,6 +299,12 @@ static void curl_multi_check_completion(BDRVCURLState *s) > > > > /* ACBs for successful messages get completed in curl_read_cb */ > > > > if (msg->data.result != CURLE_OK) { > > > > int i; > > > > + > > > > + /* Don't lose the original error message from curl, since > > > > + * it contains extra data. > > > > + */ > > > > + error_report("curl: %s", state->errmsg); > > > > + > > > > for (i = 0; i < CURL_NUM_ACB; i++) { > > > > CURLAIOCB *acb = state->acb[i]; > > > > > > > > > > Printing an error message, then returning an error code is problematic. > > > > > > It works when the caller is going to print its own error message to the > > > same destination. Callee produces a specific error message devoid of > > > context, caller produces an unspecific one with hopefully more context. > > > Better than just one of them. Worse than a single specific error with > > > context, but that can't be done with just a "return errno code" > > > interface. > > > > > > It's kind of wrong when the caller reports its own error somewhere else, > > > e.g. to a monitor. Still, when barfing extra info to stderr is the best > > > we can do, it's better than nothing. > > > > > > It's more wrong when the caller handles the error quietly. I guess > > > that's never the case here, but I can't be sure without a lot more > > > sleuthing. Perhaps Kevin or Stefan can judge this immediately. > > > > I'm not worried too much about requests made by the monitor or during > > startup. I don't like the error_report() there, but having a more > > specific error message on stderr is better than having nothing. > > > > The case that bothers me more is guest requests. Depending on the > > werror/rerror settings, this may allow the guest to flood the log file > > with curl error messages. > > Can you expand a bit on how they would do this? I can see how the > remote web server can cause a curl error (itself possibly a concern), > but not how the guest can do it. The guest can't cause it, but once the connection is down, I expect every request to fail. You don't have to have a malicious guest for filling up the log file, it just needs to be careless enough to continue trying new requests instead of offlining the device. Kevin
diff --git a/block/curl.c b/block/curl.c index 3a2b63e..2fd7c06 100644 --- a/block/curl.c +++ b/block/curl.c @@ -22,6 +22,7 @@ * THE SOFTWARE. */ #include "qemu-common.h" +#include "qemu/error-report.h" #include "block/block_int.h" #include "qapi/qmp/qbool.h" #include "qapi/qmp/qstring.h" @@ -298,6 +299,12 @@ static void curl_multi_check_completion(BDRVCURLState *s) /* ACBs for successful messages get completed in curl_read_cb */ if (msg->data.result != CURLE_OK) { int i; + + /* Don't lose the original error message from curl, since + * it contains extra data. + */ + error_report("curl: %s", state->errmsg); + for (i = 0; i < CURL_NUM_ACB; i++) { CURLAIOCB *acb = state->acb[i]; @@ -305,7 +312,7 @@ static void curl_multi_check_completion(BDRVCURLState *s) continue; } - acb->common.cb(acb->common.opaque, -EIO); + acb->common.cb(acb->common.opaque, -EPROTO); qemu_aio_unref(acb); state->acb[i] = NULL; }