diff mbox

[PULL,02/18] replay: internal functions for replay log

Message ID 1446725643-82458-3-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Nov. 5, 2015, 12:13 p.m. UTC
From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch adds functions to perform read and write operations
with replay log.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
Message-Id: <20150917162342.8676.29445.stgit@PASHA-ISP.def.inno>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 replay/Makefile.objs     |   1 +
 replay/replay-internal.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++
 replay/replay-internal.h |  46 ++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 replay/replay-internal.c
 create mode 100644 replay/replay-internal.h

Comments

Peter Maydell May 11, 2018, 9:27 a.m. UTC | #1
On 5 November 2015 at 12:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> This patch adds functions to perform read and write operations
> with replay log.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> +void replay_put_byte(uint8_t byte)
> +{
> +    if (replay_file) {
> +        putc(byte, replay_file);
> +    }
> +}

> +uint8_t replay_get_byte(void)
> +{
> +    uint8_t byte = 0;
> +    if (replay_file) {
> +        byte = getc(replay_file);
> +    }
> +    return byte;
> +}

Coverity (CID 1390576) points out that this function isn't checking
the error return from getc(). That means we could incorrectly return
255 from here and then the return value from replay_get_dword would
be 0xffffffff, which is unfortunate if the place that's using
that uses it as a loop boundary.

Incidentally, is it worth adding something to our coverity model
to tell coverity that data from replay_get_byte() is not tainted?

thanks
-- PMM
Paolo Bonzini May 11, 2018, 9:51 a.m. UTC | #2
On 11/05/2018 11:27, Peter Maydell wrote:
>> +uint8_t replay_get_byte(void)
>> +{
>> +    uint8_t byte = 0;
>> +    if (replay_file) {
>> +        byte = getc(replay_file);
>> +    }
>> +    return byte;
>> +}
> Coverity (CID 1390576) points out that this function isn't checking
> the error return from getc(). That means we could incorrectly return
> 255 from here and then the return value from replay_get_dword would
> be 0xffffffff, which is unfortunate if the place that's using
> that uses it as a loop boundary.

Thanks!  Pavel can you check it?  How is error checking done in general
for record/replay, should QEMU exit immediately?

> Incidentally, is it worth adding something to our coverity model
> to tell coverity that data from replay_get_byte() is not tainted?

Good idea.  Something like

uint8_t replay_get_byte(void)
{
     uint8_t byte;
     if (!replay_file) {
         return 0;
     }
     return byte;
}

should do.

Paolo
Pavel Dovgalyuk May 11, 2018, 9:56 a.m. UTC | #3
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 11/05/2018 11:27, Peter Maydell wrote:
> >> +uint8_t replay_get_byte(void)
> >> +{
> >> +    uint8_t byte = 0;
> >> +    if (replay_file) {
> >> +        byte = getc(replay_file);
> >> +    }
> >> +    return byte;
> >> +}
> > Coverity (CID 1390576) points out that this function isn't checking
> > the error return from getc(). That means we could incorrectly return
> > 255 from here and then the return value from replay_get_dword would
> > be 0xffffffff, which is unfortunate if the place that's using
> > that uses it as a loop boundary.
> 
> Thanks!  Pavel can you check it?  How is error checking done in general
> for record/replay, should QEMU exit immediately?

Yes, usually there is no sense continuing the replayed execution
in case of an io error. Therefore closing QEMU is fine.

Pavel Dovgalyuk
Markus Armbruster May 14, 2018, 6:34 a.m. UTC | #4
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/05/2018 11:27, Peter Maydell wrote:
>>> +uint8_t replay_get_byte(void)
>>> +{
>>> +    uint8_t byte = 0;
>>> +    if (replay_file) {
>>> +        byte = getc(replay_file);
>>> +    }
>>> +    return byte;
>>> +}
>> Coverity (CID 1390576) points out that this function isn't checking
>> the error return from getc(). That means we could incorrectly return
>> 255 from here and then the return value from replay_get_dword would
>> be 0xffffffff, which is unfortunate if the place that's using
>> that uses it as a loop boundary.
>
> Thanks!  Pavel can you check it?  How is error checking done in general
> for record/replay, should QEMU exit immediately?
>
>> Incidentally, is it worth adding something to our coverity model
>> to tell coverity that data from replay_get_byte() is not tainted?
>
> Good idea.  Something like
>
> uint8_t replay_get_byte(void)
> {
>      uint8_t byte;
>      if (!replay_file) {
>          return 0;
>      }
>      return byte;
> }
>
> should do.

Care to submit a patch?
diff mbox

Patch

diff --git a/replay/Makefile.objs b/replay/Makefile.objs
index d3f22c8..e67a932 100644
--- a/replay/Makefile.objs
+++ b/replay/Makefile.objs
@@ -1 +1,2 @@ 
 common-obj-y += replay.o
+common-obj-y += replay-internal.o
diff --git a/replay/replay-internal.c b/replay/replay-internal.c
new file mode 100644
index 0000000..04efeee
--- /dev/null
+++ b/replay/replay-internal.c
@@ -0,0 +1,155 @@ 
+/*
+ * replay-internal.c
+ *
+ * Copyright (c) 2010-2015 Institute for System Programming
+ *                         of the Russian Academy of Sciences.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "replay-internal.h"
+#include "qemu/error-report.h"
+#include "sysemu/sysemu.h"
+
+unsigned int replay_data_kind = -1;
+static unsigned int replay_has_unread_data;
+
+/* File for replay writing */
+FILE *replay_file;
+
+void replay_put_byte(uint8_t byte)
+{
+    if (replay_file) {
+        putc(byte, replay_file);
+    }
+}
+
+void replay_put_event(uint8_t event)
+{
+    replay_put_byte(event);
+}
+
+
+void replay_put_word(uint16_t word)
+{
+    replay_put_byte(word >> 8);
+    replay_put_byte(word);
+}
+
+void replay_put_dword(uint32_t dword)
+{
+    replay_put_word(dword >> 16);
+    replay_put_word(dword);
+}
+
+void replay_put_qword(int64_t qword)
+{
+    replay_put_dword(qword >> 32);
+    replay_put_dword(qword);
+}
+
+void replay_put_array(const uint8_t *buf, size_t size)
+{
+    if (replay_file) {
+        replay_put_dword(size);
+        fwrite(buf, 1, size, replay_file);
+    }
+}
+
+uint8_t replay_get_byte(void)
+{
+    uint8_t byte = 0;
+    if (replay_file) {
+        byte = getc(replay_file);
+    }
+    return byte;
+}
+
+uint16_t replay_get_word(void)
+{
+    uint16_t word = 0;
+    if (replay_file) {
+        word = replay_get_byte();
+        word = (word << 8) + replay_get_byte();
+    }
+
+    return word;
+}
+
+uint32_t replay_get_dword(void)
+{
+    uint32_t dword = 0;
+    if (replay_file) {
+        dword = replay_get_word();
+        dword = (dword << 16) + replay_get_word();
+    }
+
+    return dword;
+}
+
+int64_t replay_get_qword(void)
+{
+    int64_t qword = 0;
+    if (replay_file) {
+        qword = replay_get_dword();
+        qword = (qword << 32) + replay_get_dword();
+    }
+
+    return qword;
+}
+
+void replay_get_array(uint8_t *buf, size_t *size)
+{
+    if (replay_file) {
+        *size = replay_get_dword();
+        if (fread(buf, 1, *size, replay_file) != *size) {
+            error_report("replay read error");
+        }
+    }
+}
+
+void replay_get_array_alloc(uint8_t **buf, size_t *size)
+{
+    if (replay_file) {
+        *size = replay_get_dword();
+        *buf = g_malloc(*size);
+        if (fread(*buf, 1, *size, replay_file) != *size) {
+            error_report("replay read error");
+        }
+    }
+}
+
+void replay_check_error(void)
+{
+    if (replay_file) {
+        if (feof(replay_file)) {
+            error_report("replay file is over");
+            qemu_system_vmstop_request_prepare();
+            qemu_system_vmstop_request(RUN_STATE_PAUSED);
+        } else if (ferror(replay_file)) {
+            error_report("replay file is over or something goes wrong");
+            qemu_system_vmstop_request_prepare();
+            qemu_system_vmstop_request(RUN_STATE_INTERNAL_ERROR);
+        }
+    }
+}
+
+void replay_fetch_data_kind(void)
+{
+    if (replay_file) {
+        if (!replay_has_unread_data) {
+            replay_data_kind = replay_get_byte();
+            replay_check_error();
+            replay_has_unread_data = 1;
+        }
+    }
+}
+
+void replay_finish_event(void)
+{
+    replay_has_unread_data = 0;
+    replay_fetch_data_kind();
+}
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
new file mode 100644
index 0000000..17600de
--- /dev/null
+++ b/replay/replay-internal.h
@@ -0,0 +1,46 @@ 
+#ifndef REPLAY_INTERNAL_H
+#define REPLAY_INTERNAL_H
+
+/*
+ * replay-internal.h
+ *
+ * Copyright (c) 2010-2015 Institute for System Programming
+ *                         of the Russian Academy of Sciences.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <stdio.h>
+
+extern unsigned int replay_data_kind;
+
+/* File for replay writing */
+extern FILE *replay_file;
+
+void replay_put_byte(uint8_t byte);
+void replay_put_event(uint8_t event);
+void replay_put_word(uint16_t word);
+void replay_put_dword(uint32_t dword);
+void replay_put_qword(int64_t qword);
+void replay_put_array(const uint8_t *buf, size_t size);
+
+uint8_t replay_get_byte(void);
+uint16_t replay_get_word(void);
+uint32_t replay_get_dword(void);
+int64_t replay_get_qword(void);
+void replay_get_array(uint8_t *buf, size_t *size);
+void replay_get_array_alloc(uint8_t **buf, size_t *size);
+
+/*! Checks error status of the file. */
+void replay_check_error(void);
+
+/*! Finishes processing of the replayed event and fetches
+    the next event from the log. */
+void replay_finish_event(void);
+/*! Reads data type from the file and stores it in the
+    replay_data_kind variable. */
+void replay_fetch_data_kind(void);
+
+#endif