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);
     }
 }
 
-- 
2.49.0

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.

>       }
>   }
>

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.

- Steve

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

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

Steven Sistare <steven.sistare@oracle.com> writes:

> 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>

[...]

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

Will adjust v2 accordingly.

Thanks!

On 8/8/2025 4:08 AM, 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>

Reviewed-by: Steve Sistare <steven.sistare@oracle.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);
>       }
>   }
>