[Qemu-devel] Re: [PATCH 1/10] Refactor QEMUFile for live migration

[Anthony Liguori]

> > +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