Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
On 19.10.2017 18:18, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
[...]
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 5946ac09f0..29fff51fcf 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -15,6 +15,7 @@
>
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> +#include "qemu/error-report.h"
> #include "block/block.h"
> #include "qemu/rcu_queue.h"
> #include "qemu/sockets.h"
> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
> {
> /* TODO remove this in final patch submission */
> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable has "
> -"been replaced with -object iothread,poll-max-ns=NUM\n");
> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
> +"been replaced with -object iothread,poll-max-ns=NUM");
> exit(1);
> }
The comment in front of this code block indicates that this should
rather be removed completely. Stefan, do you agree?
> diff --git a/util/error.c b/util/error.c
> index 3efdd69162..e423368ca0 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -32,7 +32,7 @@ Error *error_fatal;
> static void error_handle_fatal(Error **errp, Error *err)
> {
> if (errp == &error_abort) {
> -fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> +error_report("Unexpected error in %s() at %s:%d:",
> err->func, err->src, err->line);
Indentation is still wrong if the statement spans more than one line :-(
Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
Am 19.10.2017 um 19:53 schrieb Thomas Huth:
> On 19.10.2017 18:18, Alistair Francis wrote:
>> Replace a large number of the fprintf(stderr, "*\n" calls with
>> error_report(). The functions were renamed with these commands and then
>> compiler issues where manually fixed.
> [...]
>> diff --git a/util/aio-posix.c b/util/aio-posix.c
>> index 5946ac09f0..29fff51fcf 100644
>> --- a/util/aio-posix.c
>> +++ b/util/aio-posix.c
>> @@ -15,6 +15,7 @@
>>
>> #include "qemu/osdep.h"
>> #include "qemu-common.h"
>> +#include "qemu/error-report.h"
>> #include "block/block.h"
>> #include "qemu/rcu_queue.h"
>> #include "qemu/sockets.h"
>> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
>> {
>> /* TODO remove this in final patch submission */
>> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
>> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable has "
>> -"been replaced with -object iothread,poll-max-ns=NUM\n");
>> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
>> +"been replaced with -object iothread,poll-max-ns=NUM");
>> exit(1);
>> }
>
> The comment in front of this code block indicates that this should
> rather be removed completely. Stefan, do you agree?
I assume you asked the other Stefan, but I think he'll agree as I do,
because it is obvious that such random debug code does not belong
into the QEMU code base.
Stefan
Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
On 19.10.2017 21:47, Stefan Weil wrote:
> Am 19.10.2017 um 19:53 schrieb Thomas Huth:
>> On 19.10.2017 18:18, Alistair Francis wrote:
>>> Replace a large number of the fprintf(stderr, "*\n" calls with
>>> error_report(). The functions were renamed with these commands and then
>>> compiler issues where manually fixed.
>> [...]
>>> diff --git a/util/aio-posix.c b/util/aio-posix.c
>>> index 5946ac09f0..29fff51fcf 100644
>>> --- a/util/aio-posix.c
>>> +++ b/util/aio-posix.c
>>> @@ -15,6 +15,7 @@
>>>
>>> #include "qemu/osdep.h"
>>> #include "qemu-common.h"
>>> +#include "qemu/error-report.h"
>>> #include "block/block.h"
>>> #include "qemu/rcu_queue.h"
>>> #include "qemu/sockets.h"
>>> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
>>> {
>>> /* TODO remove this in final patch submission */
>>> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
>>> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable has
>>> "
>>> -"been replaced with -object iothread,poll-max-ns=NUM\n");
>>> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
>>> +"been replaced with -object iothread,poll-max-ns=NUM");
>>> exit(1);
>>> }
>>
>> The comment in front of this code block indicates that this should
>> rather be removed completely. Stefan, do you agree?
>
> I assume you asked the other Stefan, but I think he'll agree as I do,
Right, the code has been introduced by this commit:
commit 4a1cba3802554a3b077d436002519ff1fb0c18bf
Author: Stefan Hajnoczi
Date: Thu Dec 1 19:26:42 2016 +
aio: add polling mode to AioContext
and apparently the environment variable has never been used in the
committed code base, so removing this code block sounds like the right
way to go.
Thomas
Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
On Fri, Oct 20, 2017 at 08:27:41AM +0200, Thomas Huth wrote:
> On 19.10.2017 21:47, Stefan Weil wrote:
> > Am 19.10.2017 um 19:53 schrieb Thomas Huth:
> >> On 19.10.2017 18:18, Alistair Francis wrote:
> >>> Replace a large number of the fprintf(stderr, "*\n" calls with
> >>> error_report(). The functions were renamed with these commands and then
> >>> compiler issues where manually fixed.
> >> [...]
> >>> diff --git a/util/aio-posix.c b/util/aio-posix.c
> >>> index 5946ac09f0..29fff51fcf 100644
> >>> --- a/util/aio-posix.c
> >>> +++ b/util/aio-posix.c
> >>> @@ -15,6 +15,7 @@
> >>>
> >>> #include "qemu/osdep.h"
> >>> #include "qemu-common.h"
> >>> +#include "qemu/error-report.h"
> >>> #include "block/block.h"
> >>> #include "qemu/rcu_queue.h"
> >>> #include "qemu/sockets.h"
> >>> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
> >>> {
> >>> /* TODO remove this in final patch submission */
> >>> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
> >>> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable
> >>> has "
> >>> -"been replaced with -object iothread,poll-max-ns=NUM\n");
> >>> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
> >>> +"been replaced with -object iothread,poll-max-ns=NUM");
> >>> exit(1);
> >>> }
> >>
> >> The comment in front of this code block indicates that this should
> >> rather be removed completely. Stefan, do you agree?
> >
> > I assume you asked the other Stefan, but I think he'll agree as I do,
>
> Right, the code has been introduced by this commit:
>
> commit 4a1cba3802554a3b077d436002519ff1fb0c18bf
> Author: Stefan Hajnoczi
> Date: Thu Dec 1 19:26:42 2016 +
>
> aio: add polling mode to AioContext
>
> and apparently the environment variable has never been used in the
> committed code base, so removing this code block sounds like the right
> way to go.
Yes, feel free to remove it.
Initially the RFC patches used the QEMU_AIO_POLL_MAX_NS environment
variable. When I wrote a proper command-line interface for this setting
I left the error to help any of the people testing this optimization
migrate their scripts to the new interface.
...Then I forgot to remove it for the final non-RFC patch which got
merged :).
Stefan
Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()
On Fri, Oct 20, 2017 at 3:47 AM, Stefan Hajnoczi wrote:
> On Fri, Oct 20, 2017 at 08:27:41AM +0200, Thomas Huth wrote:
>> On 19.10.2017 21:47, Stefan Weil wrote:
>> > Am 19.10.2017 um 19:53 schrieb Thomas Huth:
>> >> On 19.10.2017 18:18, Alistair Francis wrote:
>> >>> Replace a large number of the fprintf(stderr, "*\n" calls with
>> >>> error_report(). The functions were renamed with these commands and then
>> >>> compiler issues where manually fixed.
>> >> [...]
>> >>> diff --git a/util/aio-posix.c b/util/aio-posix.c
>> >>> index 5946ac09f0..29fff51fcf 100644
>> >>> --- a/util/aio-posix.c
>> >>> +++ b/util/aio-posix.c
>> >>> @@ -15,6 +15,7 @@
>> >>>
>> >>> #include "qemu/osdep.h"
>> >>> #include "qemu-common.h"
>> >>> +#include "qemu/error-report.h"
>> >>> #include "block/block.h"
>> >>> #include "qemu/rcu_queue.h"
>> >>> #include "qemu/sockets.h"
>> >>> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
>> >>> {
>> >>> /* TODO remove this in final patch submission */
>> >>> if (getenv("QEMU_AIO_POLL_MAX_NS")) {
>> >>> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable
>> >>> has "
>> >>> -"been replaced with -object
>> >>> iothread,poll-max-ns=NUM\n");
>> >>> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has
>> >>> "
>> >>> +"been replaced with -object iothread,poll-max-ns=NUM");
>> >>> exit(1);
>> >>> }
>> >>
>> >> The comment in front of this code block indicates that this should
>> >> rather be removed completely. Stefan, do you agree?
>> >
>> > I assume you asked the other Stefan, but I think he'll agree as I do,
>>
>> Right, the code has been introduced by this commit:
>>
>> commit 4a1cba3802554a3b077d436002519ff1fb0c18bf
>> Author: Stefan Hajnoczi
>> Date: Thu Dec 1 19:26:42 2016 +
>>
>> aio: add polling mode to AioContext
>>
>> and apparently the environment variable has never been used in the
>> committed code base, so removing this code block sounds like the right
>> way to go.
>
> Yes, feel free to remove it.
>
> Initially the RFC patches used the QEMU_AIO_POLL_MAX_NS environment
> variable. When I wrote a proper command-line interface for this setting
> I left the error to help any of the people testing this optimization
> migrate their scripts to the new interface.
>
> ...Then I forgot to remove it for the final non-RFC patch which got
> merged :).
Ok, I will remove it in the next version.
Thanks,
Alistair
>
> Stefan
