Re: [Qemu-devel] [PULL 05/22] virtio-pmem: add virtio device

From: Pankaj Gupta <pagupta@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Markus Armbruster <armbru@redhat.com>,
	David Hildenbrand <david@redhat.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [Qemu-devel] [PULL 05/22] virtio-pmem: add virtio device
Date: Thu, 11 Jul 2019 10:05:51 -0400 (EDT)	[thread overview]
Message-ID: <321746293.40974532.1562853951874.JavaMail.zimbra@redhat.com> (raw)
In-Reply-To: <CAFEAcA9=t7uK+=P3noWnP2Ga7=eKiPy9ZoDG9x=ty1P9UJgS3g@mail.gmail.com>

> >
> > From: Pankaj Gupta <pagupta@redhat.com>
> >
> > This is the implementation of virtio-pmem device. Support will require
> > machine changes for the architectures that will support it, so it will
> > not yet be compiled. It can be unlocked with VIRTIO_PMEM_SUPPORTED per
> > machine and disabled globally via VIRTIO_PMEM.
> >
> > We cannot use the "addr" property as that is already used e.g. for
> > virtio-pci/pci devices. And we will have e.g. virtio-pmem-pci as a proxy.
> > So we have to choose a different one (unfortunately). "memaddr" it is.
> > That name should ideally be used by all other virtio-* based memory
> > devices in the future.
> >     -device virtio-pmem-pci,id=p0,bus=bux0,addr=0x01,memaddr=0x1000000...
> >
> > Acked-by: Markus Armbruster <armbru@redhat.com>
> > [ QAPI bits ]
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>'-numa node,memdev
> > [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
> >   split up patches, unplug handler ]
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > Message-Id: <20190619094907.10131-2-pagupta@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> > +static void virtio_pmem_fill_device_info(const VirtIOPMEM *pmem,
> > +                                         VirtioPMEMDeviceInfo *vi)
> > +{
> > +    vi->memaddr = pmem->start;
> > +    vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
> > +    vi->memdev = object_get_canonical_path(OBJECT(pmem->memdev));
> 
> Hi; Coverity points out (CID 1403009) that when we're assigning
> vi->size we handle the "pmem->memdev is NULL" case; but we then
> pass it into object_get_canonical_path(), which unconditionally
> dereferences it and will crash if it is NULL. If this pointer
> can be NULL then we need to do something else here.

This will never be NULL. I will send below patch to pass coverity.

- vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
+ vi->size = memory_region_size(&pmem->memdev->mr);

Thanks,
Pankaj

> 
> > +}
> 
> thanks
> -- PMM
> 
> 