Re: [Qemu-devel] [PATCH v3 41/46] ivshmem: do not keep shm_fd open

[Marc-André Lureau]

Hi

----- Original Message -----
> On 15.09.2015 18:07, address@hidden wrote:
> > From: Marc-André Lureau <address@hidden>
> > 
> > Remove shm_fd from device state, closing it as early as possible to avoid
> > leaks.
> > 
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  hw/misc/ivshmem.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index 4adcac5..f9ac955 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -88,7 +88,6 @@ typedef struct IVShmemState {
> >      MemoryRegion ivshmem;
> >      uint64_t ivshmem_size; /* size of shared memory region */
> >      uint32_t ivshmem_64bit;
> > -    int shm_fd; /* shared memory file descriptor */
> 
> is it in no way useful during debugging to have access to this field?
> Or is it easily available elsewhere?

How would it be useful during debugging? Once the memory is mapped there isn't 
much you can do with it, it's just keeping a fd open, isn't it?

> 
> Ciao C.
> 
> >  
> >      Peer *peers;
> >      int nb_peers; /* how many peers we have space for */
> > @@ -235,7 +234,7 @@ static uint64_t ivshmem_io_read(void *opaque, hwaddr
> > addr,
> >  
> >          case IVPOSITION:
> >              /* return my VM ID if the memory is mapped */
> > -            if (s->shm_fd >= 0) {
> > +            if (memory_region_is_mapped(&s->ivshmem)) {
> >                  ret = s->vm_id;
> >              } else {
> >                  ret = -1;
> > @@ -356,8 +355,6 @@ static int create_shared_memory_BAR(IVShmemState *s,
> > int fd, uint8_t attr,
> >          return -1;
> >      }
> >  
> > -    s->shm_fd = fd;
> > -
> >      memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s), "ivshmem.bar2",
> >                                 s->ivshmem_size, ptr);
> >      vmstate_register_ram(&s->ivshmem, DEVICE(s));
> > @@ -535,7 +532,7 @@ static void ivshmem_read(void *opaque, const uint8_t
> > *buf, int size)
> >      if (incoming_posn == -1) {
> >          void * map_ptr;
> >  
> > -        if (s->shm_fd >= 0) {
> > +        if (memory_region_is_mapped(&s->ivshmem)) {
> >              error_report("shm already initialized");
> >              close(incoming_fd);
> >              return;
> > @@ -564,9 +561,7 @@ static void ivshmem_read(void *opaque, const uint8_t
> > *buf, int size)
> >  
> >          memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
> >  
> > -        /* only store the fd if it is successfully mapped */
> > -        s->shm_fd = incoming_fd;
> > -
> > +        close(incoming_fd);
> >          return;
> >      }
> >  
> > @@ -827,6 +822,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
> > **errp)
> >          }
> >  
> >          create_shared_memory_BAR(s, fd, attr, errp);
> > +        close(fd);
> >      }
> >  }
> >  
> > @@ -842,7 +838,7 @@ static void pci_ivshmem_exit(PCIDevice *dev)
> >          error_free(s->migration_blocker);
> >      }
> >  
> > -    if (s->shm_fd >= 0) {
> > +    if (memory_region_is_mapped(&s->ivshmem)) {
> >          void *addr = memory_region_get_ram_ptr(&s->ivshmem);
> >  
> >          vmstate_unregister_ram(&s->ivshmem, DEVICE(dev));
> > 
> 
> 
> 
>