Re: [Qemu-devel] [RFC PATCH v2 12/17] fuzz: Add fuzzer skeleton

[Stefan Hajnoczi]

On Mon, Aug 05, 2019 at 07:11:13AM +0000, Oleinik, Alexander wrote:
> diff --git a/tests/fuzz/fuzz.c b/tests/fuzz/fuzz.c
> new file mode 100644
> index 0000000000..9e03e15d7b
> --- /dev/null
> +++ b/tests/fuzz/fuzz.c
> @@ -0,0 +1,245 @@
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qemu/iov.h"
> +#include "exec/memory.h"
> +#include "exec/address-spaces.h"
> +#include "migration/qemu-file.h"
> +
> +#include "migration/qemu-file.h"
> +#include "migration/global_state.h"
> +#include "migration/savevm.h"
> +#include "tests/libqtest.h"
> +#include "migration/migration.h"
> +#include "fuzz.h"
> +#include "tests/libqos/qgraph.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <linux/userfaultfd.h>
> +#include <poll.h>
> +#include <pthread.h>
> +#include <sys/syscall.h>
> +#include <sys/types.h>
> +#include <sys/ioctl.h>

There's a lot of stuff going on here, the qemu-file.h include is
duplicated, system headers should be before application headers (see
./HACKING), and some of the headers aren't used.  Please clean these
includes up.

> +/* Save the entire vm state including RAM */
> +void save_vm_state(void)
> +{
> +    writefile = qemu_fopen_ram(&rd);
> +    vm_stop(RUN_STATE_SAVE_VM);
> +    global_state_store();
> +    qemu_savevm_state(writefile, NULL);
> +    qemu_fflush(writefile);
> +    ramfile = qemu_fopen_ro_ram(rd);
> +}
> +
> +/* Reset state by rebooting */
> +void reboot()

Please use void foo(void) in C.  Unlike C++, where void foo() is
equivalent to void foo(void), void foo() means that the arguments are
unspecified and not checked by the compiler!

> +void qtest_setup()
> +{
> +    s = qtest_fuzz_init(NULL, NULL);
> +    global_qtest = s;

Is global_qtest used by any fuzz tests?  Thomas Huth (qtest maintainer)
wants to get rid of it soon.  Perhaps it's possible to avoid it in fuzz
tests so it never needs to be introduced.

> diff --git a/tests/fuzz/fuzz.h b/tests/fuzz/fuzz.h
> new file mode 100644
> index 0000000000..46ec38d4ea
> --- /dev/null
> +++ b/tests/fuzz/fuzz.h
> @@ -0,0 +1,70 @@
> +#ifndef FUZZER_H_
> +#define FUZZER_H_

There are a bunch of global variables in this file.  It's not clear to
me yet at this point in the patch series that they need to be global...
Have you checked that they need to be global?

signature.asc
Description: PGP signature