[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 20/39] hw/i386: Introduce the microvm machine type
From: |
Sergio Lopez |
Subject: |
Re: [PULL 20/39] hw/i386: Introduce the microvm machine type |
Date: |
Tue, 12 Nov 2019 12:57:02 +0100 |
User-agent: |
mu4e 1.2.0; emacs 26.2 |
Peter Maydell <address@hidden> writes:
> On Thu, 24 Oct 2019 at 16:19, Paolo Bonzini <address@hidden> wrote:
>>
>> From: Sergio Lopez <address@hidden>
>>
>> microvm is a machine type inspired by Firecracker and constructed
>> after its machine model.
>>
>> It's a minimalist machine type without PCI nor ACPI support, designed
>> for short-lived guests. microvm also establishes a baseline for
>> benchmarking and optimizing both QEMU and guest operating systems,
>> since it is optimized for both boot time and footprint.
>
> Hi; Coverity points out a memory leak in this commit
> (CID 1407218):
>
>
>> +static void microvm_fix_kernel_cmdline(MachineState *machine)
>> +{
>> + X86MachineState *x86ms = X86_MACHINE(machine);
>> + BusState *bus;
>> + BusChild *kid;
>> + char *cmdline;
>> +
>> + /*
>> + * Find MMIO transports with attached devices, and add them to the
>> kernel
>> + * command line.
>> + *
>> + * Yes, this is a hack, but one that heavily improves the UX without
>> + * introducing any significant issues.
>> + */
>> + cmdline = g_strdup(machine->kernel_cmdline);
>
> Here we allocate memory for cmdline...
>
>> + bus = sysbus_get_default();
>> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
>> + DeviceState *dev = kid->child;
>> + ObjectClass *class = object_get_class(OBJECT(dev));
>> +
>> + if (class == object_class_by_name(TYPE_VIRTIO_MMIO)) {
>> + VirtIOMMIOProxy *mmio = VIRTIO_MMIO(OBJECT(dev));
>> + VirtioBusState *mmio_virtio_bus = &mmio->bus;
>> + BusState *mmio_bus = &mmio_virtio_bus->parent_obj;
>> +
>> + if (!QTAILQ_EMPTY(&mmio_bus->children)) {
>> + gchar *mmio_cmdline =
>> microvm_get_mmio_cmdline(mmio_bus->name);
>> + if (mmio_cmdline) {
>> + char *newcmd = g_strjoin(NULL, cmdline, mmio_cmdline,
>> NULL);
>> + g_free(mmio_cmdline);
>> + g_free(cmdline);
>> + cmdline = newcmd;
>> + }
>> + }
>> + }
>> + }
>> +
>> + fw_cfg_modify_i32(x86ms->fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(cmdline) +
>> 1);
>> + fw_cfg_modify_string(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA, cmdline);
>
> ...but fw_cfg_modify_string() takes a copy of the string it's passed,
> so we still have ownership of 'cmdline' and need to free it here
> to avoid a leak.
Ack, will send a patch ASAP.
Thanks,
Sergio.
>> +}
>
> thanks
> -- PMM