[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] more replay fixes
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] more replay fixes |
Date: |
Tue, 6 Oct 2015 14:13:52 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 4/4] events doubts, (continued)
[Qemu-devel] [PATCH 1/4] replay: generalize ptimer event to bottom halves, Paolo Bonzini, 2015/10/08
[Qemu-devel] [PATCH 3/4] why is runstate_is_running needed?, Paolo Bonzini, 2015/10/08
[Qemu-devel] [PATCH 2/4] more replay fixes, Paolo Bonzini, 2015/10/08
Re: [Qemu-devel] [RFH PATCH 0/4] record/replay fixups and doubts, Pavel Dovgaluk, 2015/10/13
Re: [Qemu-devel] [RFH PATCH 0/4] record/replay fixups and doubts, Pavel Dovgaluk, 2015/10/23