Re: [Qemu-devel] [PATCH 23/28] ahci: Adding basic functionality qtest.

[Stefan Hajnoczi]

On Mon, Jul 07, 2014 at 02:18:04PM -0400, John Snow wrote:
> +/* To help make it clear that the HBA is not a pointer to local memory. */
> +typedef void HBA;

Unused.  Please drop it or move it to the patch that uses it.

> +static void free_ahci_device(QPCIDevice *ahci)
> +{
> +    if (pcibus) {
> +        qpci_free_pc(pcibus);
> +        pcibus = NULL;
> +    }
> +
> +    /* libqos doesn't have a function for this, so free it manually */
> +    g_free(ahci);

Since the AHCI QPCIDevice comes from the QPCIBus, it would make sense to
free the AHCI device before freeing the bus.

> +}
> +
> +/*** Test Setup & Teardown ***/
> +
> +/**
> + * Launch QEMU with the given command line,
> + * and then set up interrupts and our guest malloc interface.
> + */
> +static void qtest_boot(const char *cmdline_fmt, ...)
> +{
> +    va_list ap;
> +    char *cmdline;
> +
> +    va_start(ap, cmdline_fmt);
> +    cmdline = g_strdup_vprintf(cmdline_fmt, ap);
> +    va_end(ap);
> +
> +    qtest_start(cmdline);
> +    qtest_irq_intercept_in(global_qtest, "ioapic");
> +    guest_malloc = pc_alloc_init();
> +
> +    free(cmdline);

g_strdup_vprintf() needs g_free(), not free().

> +}
> +
> +/**
> + * Tear down the QEMU instance.
> + */
> +static void qtest_shutdown(void)
> +{
> +    if (guest_malloc) {
> +        g_free(guest_malloc);
> +        guest_malloc = NULL;
> +    }

g_free(NULL) is a nop, so this is equivalent:

  g_free(guest_malloc);
  guest_malloc = NULL;

> +    qtest_end();
> +}
> +
> +/**
> + * Start a Q35 machine and bookmark a handle to the AHCI device.
> + */
> +static void ahci_boot(QPCIDevice **ahci)
> +{
> +    QPCIDevice *dev;
> +
> +    qtest_boot("-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
> +               " -M q35 "
> +               "-device ide-hd,drive=drive0 "
> +               "-global ide-hd.ver=%s",
> +               tmp_path, "testdisk", "version");
> +
> +    /* Verify that we have an AHCI device present. */
> +    dev = get_ahci_device();
> +
> +    if (ahci) {
> +        *ahci = dev;
> +    }

It would be simpler to just return the QPCIDevice.

pgp0RsPO703TW.pgp
Description: PGP signature