Re: [Qemu-devel] [PATCH 2/4] more replay fixes

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On 10/06/2015 02:00 PM, Paolo Bonzini wrote:
> 1) Compile files once
> 
> 2) Move include file from replay/replay.h to include/sysemu/replay.h.
> 
> 3) Fix Error usage
> 
> 4) cleanup timerlistgroup_deadline_ns a bit and allow clock jump
> notifiers to run
> 
> 5) move replay-user.c to stubs/
> ---

> +++ b/include/qapi/qmp/qerror.h
> @@ -107,6 +107,6 @@
>      "this feature or command is not currently supported"
>  
>  #define QERR_REPLAY_NOT_SUPPORTED \
> -    ERROR_CLASS_GENERIC_ERROR, "Record/replay feature is not supported for '%s'"
> +    "Record/replay feature is not supported for '%s'"

We should not be adding new #defines to this file.  Instead, inline the
message into the callers that do error_setg() (I see hw/bt/hci.c as the
first such caller).

> +++ b/qapi/common.json
> @@ -22,15 +22,11 @@
>  # @KVMMissingCap: the requested operation can't be fulfilled because a
>  #                 required KVM capability is missing
>  #
> -# @ReplayNotSupported: the requested feature is not supported with
> -#                      record/replay mode enabled
> -#
>  # Since: 1.2
>  ##
>  { 'enum': 'ErrorClass',
>    'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
> -            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap',
> -            'ReplayNotSupported' ] }
> +            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }

Thank you for this. We definitely do not want to be adding new error
classes without a very strong reason, and even if such classes are
added, they must properly be documented as 'Since 2.5'.

> +++ b/vl.c
> @@ -122,7 +122,7 @@ int main(int argc, char **argv)
>  #include "qapi-event.h"
>  #include "exec/semihost.h"
>  #include "crypto/init.h"
> -#include "replay/replay.h"
> +#include "sysemu/replay.h"
>  #include "qapi/qmp/qerror.h"
>  
>  #define MAX_VIRTIO_CONSOLES 1
> @@ -851,7 +851,7 @@ static void configure_rtc(QemuOpts *opts)
>          } else if (!strcmp(value, "localtime")) {
>              Error *blocker = NULL;
>              rtc_utc = 0;
> -            error_set(&blocker, ERROR_CLASS_REPLAY_NOT_SUPPORTED,
> +            error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED,
>                        "-rtc base=localtime");
>              replay_add_blocker(blocker);
>          } else {
> @@ -1258,7 +1258,7 @@ static void smp_parse(QemuOpts *opts)
>  
>      if (smp_cpus > 1 || smp_cores > 1 || smp_threads > 1) {
>          Error *blocker = NULL;
> -        error_set(&blocker, ERROR_CLASS_REPLAY_NOT_SUPPORTED, "smp");
> +        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
>          replay_add_blocker(blocker);

Okay, I see that there is more than one location with the same failure,
which is where using the #define sort of makes it nicer to guarantee a
consistent message.  But in general, use of error_setg() with a macro
that passes a %s to printf at a distance is ugly, and should be avoided
compared to just inlining the error message directly or writing a helper
method that can properly set a consistent message.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]