On Mon, 16 Sept 2024 at 08:35, Mattias Nissler <[email protected]> wrote: > > On Fri, Sep 13, 2024 at 6:47 PM Peter Maydell <[email protected]> > wrote: > > > > On Fri, 13 Sept 2024 at 16:55, Peter Xu <[email protected]> wrote: > > > > > > On Thu, Sep 12, 2024 at 03:27:55PM +0100, Peter Maydell wrote: > > > > Coverity is pretty unhappy about this trick, because it isn't able > > > > to recognise that we can figure out the address of 'bounce' > > > > from the address of 'bounce->buffer' and free it in the > > > > address_space_unmap() code, so it thinks that every use > > > > of address_space_map(), pci_dma_map(), etc, is a memory leak. > > > > We can mark all those as false positives, of course, but it got > > > > me wondering whether maybe we should have this function return > > > > a struct that has all the information address_space_unmap() > > > > needs rather than relying on it being able to figure it out > > > > from the host memory pointer... > > > > > > Indeed that sounds like a viable option. Looks like we don't have a lot > > > of > > > address_space_map() users. > > > > There's quite a few wrappers of it too, so it's a little hard to count. > > We might want to avoid the memory allocation in the common case > > by having the caller pass in an ASMapInfo struct to be filled > > in rather than having address_space_map() allocate-and-return one. > > Hm, this would work, but not only does it complicate the code > consuming address_space_map, but it also increases memory footprint (a > pointer being replaced by a struct of sizeof(BounceBuffer) if done > naively), plus there's an additional pointer indirection (I'm doubtful > whether this can be optimized away by the compiler). I haven't done > any measurements of these effects, so can't say anything definitive, > but this seems pretty costly just to appease coverity... > > Is there no way to inform coverity that a resource pointer is being > transmuted into a handle, so it can track that instead? Given that > pointer tricks like this and container_of usage is quite frequent, I > would expect coverity to have a better strategy to handle these rather > than suppressing false positive leak reports?
It's not purely that I want to appease Coverity. I also think for human readers that the current trick with passing back a pointer into host memory and relying on being able to get back to either the MR or to the bounce-buffer struct from that is pretty tricky. Would we have designed it that way if we weren't starting with the pre-existing address_space_map() function signature? thanks -- PMM
