From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KdRT3-0005ZI-TH for qemu-devel@nongnu.org; Wed, 10 Sep 2008 11:18:53 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KdRT2-0005XT-51 for qemu-devel@nongnu.org; Wed, 10 Sep 2008 11:18:53 -0400 Received: from [199.232.76.173] (port=58278 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KdRT2-0005XI-0z for qemu-devel@nongnu.org; Wed, 10 Sep 2008 11:18:52 -0400 Received: from e2.ny.us.ibm.com ([32.97.182.142]:37519) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KdRT1-0002L4-DV for qemu-devel@nongnu.org; Wed, 10 Sep 2008 11:18:51 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by e2.ny.us.ibm.com (8.13.8/8.13.8) with ESMTP id m8AFHpEo025841 for ; Wed, 10 Sep 2008 11:17:51 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m8AFHoCs200954 for ; Wed, 10 Sep 2008 11:17:50 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m8AFHnT5016250 for ; Wed, 10 Sep 2008 11:17:50 -0400 Message-ID: <48C7E4E7.7060003@us.ibm.com> Date: Wed, 10 Sep 2008 10:16:55 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1220989802-13706-1-git-send-email-aliguori@us.ibm.com> <1220989802-13706-2-git-send-email-aliguori@us.ibm.com> <5d6222a80809100738i3e3ed54fs666ca6c3225a7c63@mail.gmail.com> In-Reply-To: <5d6222a80809100738i3e3ed54fs666ca6c3225a7c63@mail.gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 1/10] Refactor QEMUFile for live migration Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Glauber Costa Cc: Chris Wright , Uri Lublin , qemu-devel@nongnu.org, kvm@vger.kernel.org >> +static int fd_put_buffer(void *opaque, const uint8_t *buf, >> + int64_t pos, int size) >> +{ >> + QEMUFileFD *s = opaque; >> + ssize_t len; >> + >> + do { >> + len = write(s->fd, buf, size); >> + } while (len == -1 && errno == EINTR); >> > > What about the len == size case ? > I don't follow what your concern is. >> + >> +static int fd_close(void *opaque) >> +{ >> + QEMUFileFD *s = opaque; >> + qemu_free(s); >> + return 0; >> +} >> > Why don't we need to do any specific callback for closing the file descriptor? > In the case of a socket, I imagine we may want to shut the socket down, for ex. > Since qemu_fopen_fd takes a previously open file descriptor, the expectation is that you're going to be able to close it yourself at some point. This worked out fine for the migration code and I think it'll also work out okay for other code. Plus, you would have to add callbacks to qemu_fopen_fd() which gets pretty nasty. >> + >> +QEMUFile *qemu_fopen_fd(int fd) >> +{ >> + QEMUFileFD *s = qemu_mallocz(sizeof(QEMUFileFD)); >> > > can't it fail? > Yeah, I should add error checking. >> -static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int64_t offset, int is_writable) >> +typedef struct QEMUFileBdrv >> +{ >> + BlockDriverState *bs; >> + int64_t base_offset; >> +} QEMUFileBdrv; >> > > Isn't it possible to abstract the differences between bdrv and file so > to have a common implementation > between them? Do you think it's worthwhile ? > It's a lot of work. QEMUFile is optimized to batch short read/write operations whereas BlockDriverState is meant to be sector based. QEMUFile is also evolving into a stream mechanism where BlockDriver will always be random access. It's certainly possible, but I don't think it's worth it at this stage. Thanks for the review! Regards, Anthony Liguori