On Thu, Nov 22, 2012 at 1:35 PM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Thu, Nov 22, 2012 at 12:58:23PM +0100, Stefan Hajnoczi wrote: >> On Thu, Nov 22, 2012 at 10:34:13AM +0100, Paolo Bonzini wrote: >> > Il 21/11/2012 19:32, Stefan Hajnoczi ha scritto: >> > > The iov_get_ptr() data returns a pointer to contiguous data within a >> > > vector. This allows the caller to manipulate data inside the vector >> > > without copying in/out using iov_from_buf()/iov_to_buf() when we know >> > > that data is contiguous within an iovec element. >> > >> > This works for you because you have a single byte to write. It would >> > not work for the SG_IO inhdr, which would need iov_to_buf(). >> >> Guilty as charged, your honor. :) >> >> Let me give a few more details about the motivation for this function: >> >> In virtio-blk-data-plane we have an iovec[] array. In the read/write >> code path we discard the inhdr/outhdr so just the data buffers are left >> in the iovec[] array. Then we can pass the iovec[] array straight to >> the Linux AIO functions. >> >> Because we're using the iovec[] array for data buffers and we're not >> allowed to make assumptions about iovec layout, we cannot use >> iov_to_buf()/iov_from_buf() at the end to fill in the status field - the >> inhdr has already been discarded from the iovec[] array. > > How about using iov_copy? > > We have exactly this problem in virtio net if we run > on host that does not support mergeable buffer header, > and we solve it by copying out the iovec. > >> Since I knew the inhdr is only 1 byte I decided against doing something >> like dynamically allocating/freeing a QEMUIOVector which could handle >> spanning iovecs. >> >> That said, I think this function is okay as-is because it works fine for >> non-virtio cases where the caller *knows* the iovec[] layout. As a >> utility function it stands on its own. >> > > My concern is these APIs are unsafe to use: you get back a pointer and > you must verify length is not too big before access. Since the iov can > be manipulated by guest this looks like a good place to put extra > safeguards. > >> > What about the following alternative API: >> > >> > void *iov_get_ptr(struct iovec *iov, unsigned int iov_cnt, >> > ssize_t offset, size_t *bytes); >> > >> > which would place the number of valid bytes (i.e. the length of the >> > remainder of the iovec entry) in *bytes? >> > >> > Also, I think that offset == iov_size(iov, iov_cnt) should be >> > acceptable, and it would be the only case in which *bytes == 0. >> >> Hmm...this may be more useful than the version I proposed since the >> caller can also use it to find out how many bytes are contiguous. >> >> Michael: Any concerns if I update the code to reflect Paolo's >> suggestion? >> >> Stefan > > I'd prefer something that actually works for all cases > rather than making callers check and handle failure, > or reason why it can't fail.
I just sent out a new version of the patch which goes whole hog and uses a QEMUIOVector to safely access virtio_blk_inhdr regardless of its size or iovec spanning. Stefan
