Re: [Qemu-devel] [RFC PATCH v8 19/21] replay: initialization and deinitialization

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/02/2015 13:59, Pavel Dovgaluk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@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.dovgaluk@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