| Message ID | 1370371954-8479-12-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 04, 2013 at 08:52:06PM +0200, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Please add a commit log with a bit more detail and documenting the motivation for change. > --- > hw/misc/vfio.c | 1 + > hw/net/vmxnet3.c | 3 +++ > hw/pci/msix.c | 26 ++++++++++++++++++++------ > hw/virtio/virtio-pci.c | 1 + > include/hw/pci/msix.h | 1 + > 5 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > index 3c0dc9f..2bcef7a 100644 > --- a/hw/misc/vfio.c > +++ b/hw/misc/vfio.c > @@ -2167,6 +2167,7 @@ static void vfio_teardown_msi(VFIODevice *vdev) > if (vdev->msix) { > msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem, > &vdev->bars[vdev->msix->pba_bar].mem); > + msix_free(&vdev->pdev); > } > } > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 5f483e7..9de2586 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -1979,6 +1979,7 @@ vmxnet3_init_msix(VMXNET3State *s) > if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) { > VMW_WRPRN("Failed to use MSI-X vectors, error %d", res); > msix_uninit(d, &s->msix_bar, &s->msix_bar); > + msix_free(d); > s->msix_used = false; > } else { > s->msix_used = true; > @@ -1996,6 +1997,8 @@ vmxnet3_cleanup_msix(VMXNET3State *s) > msix_vector_unuse(d, VMXNET3_MAX_INTRS); > msix_uninit(d, &s->msix_bar, &s->msix_bar); > } > + > + msix_free(d); > } > > #define VMXNET3_MSI_NUM_VECTORS (1) > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index 6da75ec..be96b06 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -274,6 +274,10 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, > dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > MSIX_MASKALL_MASK; > > + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { > + msix_free(dev); > + } > + > dev->msix_table = g_malloc0(table_size); > dev->msix_pba = g_malloc0(pba_size); > dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); Wow msix_init calls msix_free, and not on error path? What's going on here? > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) > msix_free_irq_entries(dev); > dev->msix_entries_nr = 0; > memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); > - memory_region_destroy(&dev->msix_pba_mmio); > - g_free(dev->msix_pba); > - dev->msix_pba = NULL; > memory_region_del_subregion(table_bar, &dev->msix_table_mmio); > - memory_region_destroy(&dev->msix_table_mmio); > - g_free(dev->msix_table); > + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > +} > + > +void msix_free(PCIDevice *dev) > +{ > + if (dev->msix_pba) { > + memory_region_destroy(&dev->msix_pba_mmio); > + g_free(dev->msix_pba); > + } > + dev->msix_pba = NULL; > + > + if (dev->msix_table) { > + memory_region_destroy(&dev->msix_table_mmio); > + g_free(dev->msix_table); > + } > dev->msix_table = NULL; > + > g_free(dev->msix_entry_used); > dev->msix_entry_used = NULL; > - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > } > > void msix_uninit_exclusive_bar(PCIDevice *dev) As long as we had init and uninit, it was mostly self-documenting. Now, there are two cleanup functions, so please add documentation. > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 444b71a..cbfec6b 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -998,6 +998,7 @@ static void virtio_pci_exit(PCIDevice *pci_dev) > virtio_pci_stop_ioeventfd(proxy); > memory_region_destroy(&proxy->bar); > msix_uninit_exclusive_bar(pci_dev); > + msix_free(pci_dev); > } > > static void virtio_pci_reset(DeviceState *qdev) > diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h > index 954d82b..34e3ba2 100644 > --- a/include/hw/pci/msix.h > +++ b/include/hw/pci/msix.h > @@ -18,6 +18,7 @@ void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len); > void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, > MemoryRegion *pba_bar); > void msix_uninit_exclusive_bar(PCIDevice *dev); > +void msix_free(PCIDevice *dev); > > unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > -- > 1.8.1.4 >
Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: >> > + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { >> > + msix_free(dev); >> > + } >> > + >> > dev->msix_table = g_malloc0(table_size); >> > dev->msix_pba = g_malloc0(pba_size); >> > dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); > Wow msix_init calls msix_free, and not on error path? > What's going on here? I wasn't too sure that you could get here only with NULL msix_table/pba/entry_used and wanted to protect against leaks. I'll change it to an assertion. >> > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) >> > msix_free_irq_entries(dev); >> > dev->msix_entries_nr = 0; >> > memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); >> > - memory_region_destroy(&dev->msix_pba_mmio); >> > - g_free(dev->msix_pba); >> > - dev->msix_pba = NULL; >> > memory_region_del_subregion(table_bar, &dev->msix_table_mmio); >> > - memory_region_destroy(&dev->msix_table_mmio); >> > - g_free(dev->msix_table); >> > + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; >> > +} >> > + >> > +void msix_free(PCIDevice *dev) >> > +{ >> > + if (dev->msix_pba) { >> > + memory_region_destroy(&dev->msix_pba_mmio); >> > + g_free(dev->msix_pba); >> > + } >> > + dev->msix_pba = NULL; >> > + >> > + if (dev->msix_table) { >> > + memory_region_destroy(&dev->msix_table_mmio); >> > + g_free(dev->msix_table); >> > + } >> > dev->msix_table = NULL; >> > + >> > g_free(dev->msix_entry_used); >> > dev->msix_entry_used = NULL; >> > - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; >> > } >> > >> > void msix_uninit_exclusive_bar(PCIDevice *dev) > As long as we had init and uninit, it was mostly > self-documenting. > Now, there are two cleanup functions, so please add documentation. Yes, will do. Paolo
On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: > Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: > >> > + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { > >> > + msix_free(dev); > >> > + } > >> > + > >> > dev->msix_table = g_malloc0(table_size); > >> > dev->msix_pba = g_malloc0(pba_size); > >> > dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); > > Wow msix_init calls msix_free, and not on error path? > > What's going on here? > > I wasn't too sure that you could get here only with NULL > msix_table/pba/entry_used and wanted to protect against leaks. I'll > change it to an assertion. I don't think we should require users allocate all memory with g_malloc0. So no assertion either. If there's a leak there was always a leak, let's focus on the API change in this series, OK? > >> > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) > >> > msix_free_irq_entries(dev); > >> > dev->msix_entries_nr = 0; > >> > memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); > >> > - memory_region_destroy(&dev->msix_pba_mmio); > >> > - g_free(dev->msix_pba); > >> > - dev->msix_pba = NULL; > >> > memory_region_del_subregion(table_bar, &dev->msix_table_mmio); > >> > - memory_region_destroy(&dev->msix_table_mmio); > >> > - g_free(dev->msix_table); > >> > + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > >> > +} > >> > + > >> > +void msix_free(PCIDevice *dev) > >> > +{ > >> > + if (dev->msix_pba) { > >> > + memory_region_destroy(&dev->msix_pba_mmio); > >> > + g_free(dev->msix_pba); > >> > + } > >> > + dev->msix_pba = NULL; > >> > + > >> > + if (dev->msix_table) { > >> > + memory_region_destroy(&dev->msix_table_mmio); > >> > + g_free(dev->msix_table); > >> > + } > >> > dev->msix_table = NULL; > >> > + > >> > g_free(dev->msix_entry_used); > >> > dev->msix_entry_used = NULL; > >> > - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > >> > } > >> > > >> > void msix_uninit_exclusive_bar(PCIDevice *dev) > > As long as we had init and uninit, it was mostly > > self-documenting. > > Now, there are two cleanup functions, so please add documentation. > > Yes, will do. > > Paolo
Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto: > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: >>>>> + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { >>>>> + msix_free(dev); >>>>> + } >>>>> + >>>>> dev->msix_table = g_malloc0(table_size); >>>>> dev->msix_pba = g_malloc0(pba_size); >>>>> dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); >>> Wow msix_init calls msix_free, and not on error path? >>> What's going on here? >> >> I wasn't too sure that you could get here only with NULL >> msix_table/pba/entry_used and wanted to protect against leaks. I'll >> change it to an assertion. > > I don't think we should require users allocate all memory with g_malloc0. > So no assertion either. Assertion that is is NULL, followed by g_malloc0? > If there's a leak there was always a leak No, there wasn't because msix_uninit would have freed the memory. That is, msix_init msix_uninit msix_init msix_uninit had no leak. Instead, now msix_free is going to be called just once, right before freeing the object itself: msix_init msix_uninit msix_init *** msix_uninit msix_free and will have a leak at ***. I don't think this can happen, unrealize should never be followed by another realize right now, but perhaps in the future it will be if we implement something like "device_poweroff" and "device_poweron". Paolo , let's focus on the > API change in this series, OK? > >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) >>>>> msix_free_irq_entries(dev); >>>>> dev->msix_entries_nr = 0; >>>>> memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); >>>>> - memory_region_destroy(&dev->msix_pba_mmio); >>>>> - g_free(dev->msix_pba); >>>>> - dev->msix_pba = NULL; >>>>> memory_region_del_subregion(table_bar, &dev->msix_table_mmio); >>>>> - memory_region_destroy(&dev->msix_table_mmio); >>>>> - g_free(dev->msix_table); >>>>> + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; >>>>> +} >>>>> + >>>>> +void msix_free(PCIDevice *dev) >>>>> +{ >>>>> + if (dev->msix_pba) { >>>>> + memory_region_destroy(&dev->msix_pba_mmio); >>>>> + g_free(dev->msix_pba); >>>>> + } >>>>> + dev->msix_pba = NULL; >>>>> + >>>>> + if (dev->msix_table) { >>>>> + memory_region_destroy(&dev->msix_table_mmio); >>>>> + g_free(dev->msix_table); >>>>> + } >>>>> dev->msix_table = NULL; >>>>> + >>>>> g_free(dev->msix_entry_used); >>>>> dev->msix_entry_used = NULL; >>>>> - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; >>>>> } >>>>> >>>>> void msix_uninit_exclusive_bar(PCIDevice *dev) >>> As long as we had init and uninit, it was mostly >>> self-documenting. >>> Now, there are two cleanup functions, so please add documentation. >> >> Yes, will do. >> >> Paolo > >
On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote: > Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto: > > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: > >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: > >>>>> + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { > >>>>> + msix_free(dev); > >>>>> + } > >>>>> + > >>>>> dev->msix_table = g_malloc0(table_size); > >>>>> dev->msix_pba = g_malloc0(pba_size); > >>>>> dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); > >>> Wow msix_init calls msix_free, and not on error path? > >>> What's going on here? > >> > >> I wasn't too sure that you could get here only with NULL > >> msix_table/pba/entry_used and wanted to protect against leaks. I'll > >> change it to an assertion. > > > > I don't think we should require users allocate all memory with g_malloc0. > > So no assertion either. > > Assertion that is is NULL, followed by g_malloc0? No because who sets it to NULL the first time? msix_init just started. > > If there's a leak there was always a leak > > No, there wasn't because msix_uninit would have freed the memory. That is, > > msix_init > msix_uninit > msix_init > msix_uninit > > had no leak. Instead, now msix_free is going to be called just once, > right before freeing the object itself: > > msix_init > msix_uninit > msix_init *** > msix_uninit > msix_free > > and will have a leak at ***. Yes. And this looks completely sane from outside, so this is a bad API. The way to fix it is not with asserts in code, we need a good API: alloc/free init/uninit ... The problem apparently starts in generic code, let's fix it there? > I don't think this can happen, unrealize > should never be followed by another realize right now, This is not an msix specific problem, I don't think msix should debug generic core - this will just lead to proliferation of asserts. This really should be documented prominently in generic code. Also how about some asserts in generic code making sure ordering is sane? > but perhaps in > the future it will be if we implement something like "device_poweroff" > and "device_poweron". > > Paolo > , let's focus on the > > API change in this series, OK? > > > >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) > >>>>> msix_free_irq_entries(dev); > >>>>> dev->msix_entries_nr = 0; > >>>>> memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); > >>>>> - memory_region_destroy(&dev->msix_pba_mmio); > >>>>> - g_free(dev->msix_pba); > >>>>> - dev->msix_pba = NULL; > >>>>> memory_region_del_subregion(table_bar, &dev->msix_table_mmio); > >>>>> - memory_region_destroy(&dev->msix_table_mmio); > >>>>> - g_free(dev->msix_table); > >>>>> + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > >>>>> +} > >>>>> + > >>>>> +void msix_free(PCIDevice *dev) > >>>>> +{ > >>>>> + if (dev->msix_pba) { > >>>>> + memory_region_destroy(&dev->msix_pba_mmio); > >>>>> + g_free(dev->msix_pba); > >>>>> + } > >>>>> + dev->msix_pba = NULL; > >>>>> + > >>>>> + if (dev->msix_table) { > >>>>> + memory_region_destroy(&dev->msix_table_mmio); > >>>>> + g_free(dev->msix_table); > >>>>> + } > >>>>> dev->msix_table = NULL; > >>>>> + > >>>>> g_free(dev->msix_entry_used); > >>>>> dev->msix_entry_used = NULL; > >>>>> - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > >>>>> } > >>>>> > >>>>> void msix_uninit_exclusive_bar(PCIDevice *dev) > >>> As long as we had init and uninit, it was mostly > >>> self-documenting. > >>> Now, there are two cleanup functions, so please add documentation. > >> > >> Yes, will do. > >> > >> Paolo > > > >
Il 05/06/2013 06:32, Michael S. Tsirkin ha scritto: > On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote: >> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto: >>> On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: >>>> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: >>>>>>> + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { >>>>>>> + msix_free(dev); >>>>>>> + } >>>>>>> + >>>>>>> dev->msix_table = g_malloc0(table_size); >>>>>>> dev->msix_pba = g_malloc0(pba_size); >>>>>>> dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); >>>>> Wow msix_init calls msix_free, and not on error path? >>>>> What's going on here? >>>> >>>> I wasn't too sure that you could get here only with NULL >>>> msix_table/pba/entry_used and wanted to protect against leaks. I'll >>>> change it to an assertion. >>> >>> I don't think we should require users allocate all memory with g_malloc0. >>> So no assertion either. >> >> Assertion that is is NULL, followed by g_malloc0? > > No because who sets it to NULL the first time? > msix_init just started. When an object is created, it is all-zeroed. >>> If there's a leak there was always a leak >> >> No, there wasn't because msix_uninit would have freed the memory. That is, >> >> msix_init >> msix_uninit >> msix_init >> msix_uninit >> >> had no leak. Instead, now msix_free is going to be called just once, >> right before freeing the object itself: >> >> msix_init >> msix_uninit >> msix_init *** >> msix_uninit >> msix_free >> >> and will have a leak at ***. > > Yes. And this looks completely sane from outside, > so this is a bad API. > The way to fix it is not with asserts in code, we need a good API: > alloc/free init/uninit ... Can't, because table_size/pba_size is not available at init time (e.g. for VFIO not until the host BARs are processed). What about using g_realloc + memset? Paolo
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c index 3c0dc9f..2bcef7a 100644 --- a/hw/misc/vfio.c +++ b/hw/misc/vfio.c @@ -2167,6 +2167,7 @@ static void vfio_teardown_msi(VFIODevice *vdev) if (vdev->msix) { msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem, &vdev->bars[vdev->msix->pba_bar].mem); + msix_free(&vdev->pdev); } } diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 5f483e7..9de2586 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -1979,6 +1979,7 @@ vmxnet3_init_msix(VMXNET3State *s) if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) { VMW_WRPRN("Failed to use MSI-X vectors, error %d", res); msix_uninit(d, &s->msix_bar, &s->msix_bar); + msix_free(d); s->msix_used = false; } else { s->msix_used = true; @@ -1996,6 +1997,8 @@ vmxnet3_cleanup_msix(VMXNET3State *s) msix_vector_unuse(d, VMXNET3_MAX_INTRS); msix_uninit(d, &s->msix_bar, &s->msix_bar); } + + msix_free(d); } #define VMXNET3_MSI_NUM_VECTORS (1) diff --git a/hw/pci/msix.c b/hw/pci/msix.c index 6da75ec..be96b06 100644 --- a/hw/pci/msix.c +++ b/hw/pci/msix.c @@ -274,6 +274,10 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | MSIX_MASKALL_MASK; + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { + msix_free(dev); + } + dev->msix_table = g_malloc0(table_size); dev->msix_pba = g_malloc0(pba_size); dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) msix_free_irq_entries(dev); dev->msix_entries_nr = 0; memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); - memory_region_destroy(&dev->msix_pba_mmio); - g_free(dev->msix_pba); - dev->msix_pba = NULL; memory_region_del_subregion(table_bar, &dev->msix_table_mmio); - memory_region_destroy(&dev->msix_table_mmio); - g_free(dev->msix_table); + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; +} + +void msix_free(PCIDevice *dev) +{ + if (dev->msix_pba) { + memory_region_destroy(&dev->msix_pba_mmio); + g_free(dev->msix_pba); + } + dev->msix_pba = NULL; + + if (dev->msix_table) { + memory_region_destroy(&dev->msix_table_mmio); + g_free(dev->msix_table); + } dev->msix_table = NULL; + g_free(dev->msix_entry_used); dev->msix_entry_used = NULL; - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; } void msix_uninit_exclusive_bar(PCIDevice *dev) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 444b71a..cbfec6b 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -998,6 +998,7 @@ static void virtio_pci_exit(PCIDevice *pci_dev) virtio_pci_stop_ioeventfd(proxy); memory_region_destroy(&proxy->bar); msix_uninit_exclusive_bar(pci_dev); + msix_free(pci_dev); } static void virtio_pci_reset(DeviceState *qdev) diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h index 954d82b..34e3ba2 100644 --- a/include/hw/pci/msix.h +++ b/include/hw/pci/msix.h @@ -18,6 +18,7 @@ void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len); void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar); void msix_uninit_exclusive_bar(PCIDevice *dev); +void msix_free(PCIDevice *dev); unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/misc/vfio.c | 1 + hw/net/vmxnet3.c | 3 +++ hw/pci/msix.c | 26 ++++++++++++++++++++------ hw/virtio/virtio-pci.c | 1 + include/hw/pci/msix.h | 1 + 5 files changed, 26 insertions(+), 6 deletions(-)