Re: [PATCH 04/12] migration/cpr: Clean up error reporting in cpr_resave_fd()

[Steven Sistare <steven.sistare@oracle.com>]

On 8/8/2025 10:43 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
> 
>> On 8/8/2025 9:55 AM, Philippe Mathieu-Daudé wrote:
>>> On 8/8/25 10:08, Markus Armbruster wrote:
>>>> qapi/error.h advises:
>>>>
>>>>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>>>>   * exit(), because that's more obvious.
>>>>
>>>> Do that.
>>>>
>>>> The error message starts with "internal error: ", so maybe this should
>>>> assert() instead.
>>>>
>>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>   migration/cpr.c | 9 +++++----
>>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/migration/cpr.c b/migration/cpr.c
>>>> index 42ad0b0d50..908bcf83b2 100644
>>>> --- a/migration/cpr.c
>>>> +++ b/migration/cpr.c
>>>> @@ -7,6 +7,7 @@
>>>>   
>>>>   #include "qemu/osdep.h"
>>>>   #include "qapi/error.h"
>>>> +#include "qemu/error-report.h"
>>>>   #include "hw/vfio/vfio-device.h"
>>>>   #include "migration/cpr.h"
>>>>   #include "migration/misc.h"
>>>> @@ -100,10 +101,10 @@ void cpr_resave_fd(const char *name, int id, int fd)
>>>>       if (old_fd < 0) {
>>>>           cpr_save_fd(name, id, fd);
>>>>       } else if (old_fd != fd) {
>>>> -        error_setg(&error_fatal,
>>>> -                   "internal error: cpr fd '%s' id %d value %d "
>>>> -                   "already saved with a different value %d",
>>>> -                   name, id, fd, old_fd);
>>>> +        error_report("internal error: cpr fd '%s' id %d value %d "
>>>> +                     "already saved with a different value %d",
>>>> +                     name, id, fd, old_fd);
>>>> +        exit(1);
>>>
>>> My 2 cents, I'm not sure this information is more helpful than a plain
>>> assertion (at least for users). No objection for this change.
>>
>> The message gives more information.  It has helped me debug
>> problems in the past, in concert with enabling cpr traces.
> 
> Is it a programming error?

Yes.

> If no, then "internal error: " is wrong.
> 
> If yes, then exit(1) is wrong.  I'd use assert() myself, but you're the
> maintainer here, and if you want this message rather than the one
> assert() gives you for free, we just replace exit(1) by abort() or
> assert(0) or, if we're feeling particularly fancy
> g_assert_not_reached().

I would like the full message.
I have no preference on how to exit.

- Steve