qemu-pr-helper -v suppresses errors, isn't that weird?

qemu-pr-helper -v suppresses errors, isn't that weird?

[Markus Armbruster]

prh_co_entry() reports reports errors reading requests / writing
responses only when @verbose (command line -v); relevant code appended
for you convenience.

Sure these are *errors*?  The program recovers and continues, and this
is deemed normal enough to inform the user only when he specifically
asks for it.  Yet when we inform, we format it as an error.  Should we
tune it down to warnings?


static void coroutine_fn prh_co_entry(void *opaque)
{
    [...]
    while (atomic_read(&state) == RUNNING) {
        [...]
        sz = prh_read_request(client, &req, &resp, &local_err);
        if (sz < 0) {
            break;
        }
        [...]
        if (prh_write_response(client, &req, &resp, &local_err) < 0) {
            break;
        }
    }
    if (local_err) {
        if (verbose == 0) {
            error_free(local_err);
        } else {
            error_report_err(local_err);
        }
    }

out:
    qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc));
    object_unref(OBJECT(client->ioc));
    g_free(client);
}

Re: qemu-pr-helper -v suppresses errors, isn't that weird?

[Paolo Bonzini]

On 18/06/20 07:32, Markus Armbruster wrote:
> prh_co_entry() reports reports errors reading requests / writing
> responses only when @verbose (command line -v); relevant code appended
> for you convenience.
> 
> Sure these are *errors*?  The program recovers and continues, and this
> is deemed normal enough to inform the user only when he specifically
> asks for it.  Yet when we inform, we format it as an error.  Should we
> tune it down to warnings?

They are errors, but they're errors in the client rather than in
qemu-pr-helper.c itself.

Paolo

> 
> static void coroutine_fn prh_co_entry(void *opaque)
> {
>     [...]
>     while (atomic_read(&state) == RUNNING) {
>         [...]
>         sz = prh_read_request(client, &req, &resp, &local_err);
>         if (sz < 0) {
>             break;
>         }
>         [...]
>         if (prh_write_response(client, &req, &resp, &local_err) < 0) {
>             break;
>         }
>     }
>     if (local_err) {
>         if (verbose == 0) {
>             error_free(local_err);
>         } else {
>             error_report_err(local_err);
>         }
>     }
> 
> out:
>     qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc));
>     object_unref(OBJECT(client->ioc));
>     g_free(client);
> }
> 

Re: qemu-pr-helper -v suppresses errors, isn't that weird?

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 18/06/20 07:32, Markus Armbruster wrote:
>> prh_co_entry() reports reports errors reading requests / writing
>> responses only when @verbose (command line -v); relevant code appended
>> for you convenience.
>> 
>> Sure these are *errors*?  The program recovers and continues, and this
>> is deemed normal enough to inform the user only when he specifically
>> asks for it.  Yet when we inform, we format it as an error.  Should we
>> tune it down to warnings?
>
> They are errors, but they're errors in the client rather than in
> qemu-pr-helper.c itself.

The error message makes it look like the error was in qemu-pr-helper.

Also, continuing after reporting an error as if nothing happened smells
suspiciously.  Even when it's not wrong, it's prone to catch a wary
reader's eye.

What do you think about something like

    warn_reportf_err(local_err, "client trouble: ");

Yes, "client trouble" isn't so hot, but I lack the understanding to do
better.

> Paolo
>
>> 
>> static void coroutine_fn prh_co_entry(void *opaque)
>> {
>>     [...]
>>     while (atomic_read(&state) == RUNNING) {
>>         [...]
>>         sz = prh_read_request(client, &req, &resp, &local_err);
>>         if (sz < 0) {
>>             break;
>>         }
>>         [...]
>>         if (prh_write_response(client, &req, &resp, &local_err) < 0) {
>>             break;
>>         }
>>     }
>>     if (local_err) {
>>         if (verbose == 0) {
>>             error_free(local_err);
>>         } else {
>>             error_report_err(local_err);
>>         }
>>     }
>> 
>> out:
>>     qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc));
>>     object_unref(OBJECT(client->ioc));
>>     g_free(client);
>> }
>>