On 09/02/2015 13:59, Pavel Dovgaluk wrote:
>> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
>> On 22/01/2015 09:53, Pavel Dovgalyuk wrote:
>>> This patch introduces the functions for enabling the record/replay and for
>>> freeing the resources when simulator closes.
>>>
>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru>
>>
>>> };
>>>
>>> /* Asynchronous events IDs */
>>> diff --git a/replay/replay.c b/replay/replay.c
>>> index 7c4a801..fa738c2 100755
>>> --- a/replay/replay.c
>>> +++ b/replay/replay.c
>>> @@ -15,9 +15,18 @@
>>> #include "qemu/timer.h"
>>> #include "sysemu/sysemu.h"
>>>
>>> +/* Current version of the replay mechanism.
>>> + Increase it when file format changes. */
>>> +#define REPLAY_VERSION 0xe02002
>>
>> Any meaning? Perhaps make it 'QRR\0' or something identifiable?
>
> Version id has no special meaning, but has to be distinct from other RR
> versions.
> When format of the log file changes we have to update version id to prevent
> reading incompatible files.
>
>>> + replay_file = fopen(fname, fmode);
>>> + if (replay_file == NULL) {
>>> + fprintf(stderr, "Replay: open %s: %s\n", fname, strerror(errno));
>>> + exit(1);
>>> + }
>>> +
>>> + replay_filename = g_strdup(fname);
>>> +
>>> + replay_mode = mode;
>>> + replay_has_unread_data = 0;
>>> + replay_data_kind = -1;
>>> + replay_state.instructions_count = 0;
>>> + replay_state.current_step = 0;
>>> +
>>> + /* skip file header for RECORD and check it for PLAY */
>>> + if (replay_mode == REPLAY_MODE_RECORD) {
>>> + fseek(replay_file, HEADER_SIZE, SEEK_SET);
>>
>> Why can't you write the header here?
>
> We don't write header here to detect broken log files at the replay stage.
I guess this is also a provision for something that you'll do in the future.
>>> + replay_save_instructions();
>>> +
>>> + /* finalize the file */
>>> + if (replay_file) {
>>> + if (replay_mode == REPLAY_MODE_RECORD) {
>>> + uint64_t offset = 0;
>>> + /* write end event */
>>> + replay_put_event(EVENT_END);
>>> +
>>> + /* write header */
>>> + fseek(replay_file, 0, SEEK_SET);
>>> + replay_put_dword(REPLAY_VERSION);
>>> + replay_put_qword(offset);
>>
>> Why is this done here? You're writing a constant zero.
>
> Right, this offset is the field reserved for snapshots table.
> It will be introduced in other patches.
Okay, then please add comments.
Paolo
