Re: [Qemu-devel] [PATCH v3] Move File operations to qemu-file.c

[Joel Schopp <jschopp@linux.vnet.ibm.com>]

>> -util-obj-y = util/ qobject/ qapi/ trace/
>> +util-obj-y = util/ qobject/ qapi/ trace/ qemu-file.o
>
> Please either move it to util/ (and the include file to
> include/qemu/file.h), or leave it in common-obj-y.  I prefer the former,
> since as a rule of thumb util-obj-y includes code that should be easy to
> unit-test.

I like that suggestion.  I'll move qemu-file.c to the util directory.

I'd rather not move the include file as it seems to be fine where it is 
and there are about 6000 places it is included that would have to be 
updated as well.  Moving the unmodified file locations doesn't gain 
anything functional.

>> +
>> +/**
>> + * Yield until a file descriptor becomes readable
>> + *
>> + * Note that this function clobbers the handlers for the file descriptor.
>> + */
>> +static void coroutine_fn yield_until_fd_readable(int fd)
>> +{
>> +    FDYieldUntilData data;
>> +
>> +    assert(qemu_in_coroutine());
>> +    data.co = qemu_coroutine_self();
>> +    data.fd = fd;
>> +    qemu_set_fd_handler(fd, fd_coroutine_enter, NULL, &data);
>> +    qemu_coroutine_yield();
>> +}
>
> Coroutines are not part of libqemuutil.a, so you should be getting
> missing symbols here; I'm surprised that the patch successfully passes
> "make check" though I may be missing something.

It does pass make check.  I'd rather move all the file related stuff out 
of migration.

>
> Everything from here down to qemu_fopen_bdrv (included) should remain in
> savevm.c:

I like these broken out better, but I don't actually need them so I'll 
go ahead and leave them where they are for now on your suggestion.

>
>> +static int block_put_buffer(void *opaque, const uint8_t *buf,
>> +                           int64_t pos, int size)
>> +{
>> +    bdrv_save_vmstate(opaque, buf, pos, size);
>> +    return size;
>> +}
>> +
>> +static int block_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>> +{
>> +    return bdrv_load_vmstate(opaque, buf, pos, size);
>> +}
>> +
>> +static int bdrv_fclose(void *opaque)
>> +{
>> +    return bdrv_flush(opaque);
>> +}
>> +
>> +static const QEMUFileOps bdrv_read_ops = {
>> +    .get_buffer = block_get_buffer,
>> +    .close =      bdrv_fclose
>> +};
>> +
>> +static const QEMUFileOps bdrv_write_ops = {
>> +    .put_buffer = block_put_buffer,
>> +    .close =      bdrv_fclose
>> +};
>> +
>> +QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
>> +{
>> +    if (is_writable)
>> +        return qemu_fopen_ops(bs, &bdrv_write_ops);
>> +    return qemu_fopen_ops(bs, &bdrv_read_ops);
>> +}