From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <aliguori@us.ibm.com>
Subject: Re: [PATCH 1/10] Refactor QEMUFile for live migration
Date: Wed, 10 Sep 2008 10:16:55 -0500
Message-ID: <48C7E4E7.7060003@us.ibm.com>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org,
	Uri Lublin <uri.lublin@qumranet.com>,
	Chris Wright <chrisw@redhat.com>
To: Glauber Costa <glommer@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from e1.ny.us.ibm.com ([32.97.182.141]:41898 "EHLO e1.ny.us.ibm.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751078AbYIJPR5 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 10 Sep 2008 11:17:57 -0400
Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234])
	by e1.ny.us.ibm.com (8.13.8/8.13.8) with ESMTP id m8AFHql7002258
	for <kvm@vger.kernel.org>; Wed, 10 Sep 2008 11:17:52 -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 m8AFHqJh165188
	for <kvm@vger.kernel.org>; Wed, 10 Sep 2008 11:17:52 -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 m8AFHnTB016250
	for <kvm@vger.kernel.org>; Wed, 10 Sep 2008 11:17:51 -0400
In-Reply-To: <5d6222a80809100738i3e3ed54fs666ca6c3225a7c63@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <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