Re: [PATCH RESEND v6 15/36] multi-process: setup memory manager for remote device

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:50PM -0700, address@hidden wrote:
> diff --git a/exec.c b/exec.c
> index 5b1e414099..1e02e00f00 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2371,6 +2371,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
> MemoryRegion *mr,
>  
>      return block;
>  }
> +
> +void qemu_ram_init_from_fd(MemoryRegion *mr, int fd, uint64_t size,

This looks like a memory_region_init_*() function, not a qemu_ram_*()
function. Why is it being added to exec.c instead of memory.c?

> +                           ram_addr_t offset, Error **errp)

qemu_ram_alloc_from_fd()'s offset argument has the off_t type. Why is
ram_addr_t used here?

> +typedef struct {
> +    hwaddr gpas[REMOTE_MAX_FDS];
> +    uint64_t sizes[REMOTE_MAX_FDS];
> +    ram_addr_t offsets[REMOTE_MAX_FDS];

Should this be off_t because it's the file offset, not a RAMBlock
address?

> +} sync_sysmem_msg_t;

QEMU coding style would name this struct SyncSysMemMsg.

> +void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
> +{
> +    sync_sysmem_msg_t *sysmem_info = &msg->data1.sync_sysmem;
> +    MemoryRegion *sysmem, *subregion, *next;
> +    Error *local_err = NULL;
> +    int region;
> +
> +    sysmem = get_system_memory();
> +
> +    qemu_mutex_lock_iothread();
> +
> +    memory_region_transaction_begin();
> +
> +    QTAILQ_FOREACH_SAFE(subregion, &sysmem->subregions, subregions_link, 
> next) {
> +        if (subregion->ram) {
> +            memory_region_del_subregion(sysmem, subregion);
> +            qemu_ram_free(subregion->ram_block);

subregion is leaked. qemu_ram_free() shouldn't be called directly. It's
normally called from the MemoryRegion->destructor function but that
wasn't set up by qemu_ram_init_from_fd().

Please check how MemoryRegion lifecycles should work, update
qemu_ram_init_from_fd(), and update this function to avoid leaks.

> +        }
> +    }
> +
> +    for (region = 0; region < msg->num_fds; region++) {
> +        subregion = g_new(MemoryRegion, 1);
> +        qemu_ram_init_from_fd(subregion, msg->fds[region],
> +                              sysmem_info->sizes[region],
> +                              sysmem_info->offsets[region], &local_err);

Where are is msg->fds[region] closed?

signature.asc
Description: PGP signature