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

[Markus Armbruster <armbru@redhat.com>]

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?

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().