Re: [Qemu-devel] [RFC] introduce a dynamic library to expose qemu block API

["Daniel P. Berrange" <berrange@redhat.com>]

On Wed, Jul 18, 2012 at 04:51:03PM +0800, Wenchao Xia wrote:
>   Hi, following is API draft, prototypes were taken from qemu/block.h,
> and the API prefix is changed frpm bdrv to qbdrvs, to declare related
> object is BlockDriverState, not BlockDriver. One issue here is it may
> require include block_int.h, which is not LGPL2 licensed yet.
>   API format is kept mostly the same with qemu generic block layer, to
> make it easier for implement, and easier to make qemu migrate on it if
> possible.


How is error reporting dealt with, and what is the intent around
thread safety of the APIs ?  I'd like to see a fully thread safe
API - multiple threads can use the same 'BlockDriverState *'
concurrently, and thread-local error reporting.

> 
> 
> /* structure init and uninit */
> BlockDriverState *qbdrvs_new(const char *device_name);
> void qbdrvs_delete(BlockDriverState *bs);
> 
> 
> /* file open and close */
> int qbdrvs_open(BlockDriverState *bs, const char *filename, int flags,
>               BlockDriver *drv);
> void qbdrvs_close(BlockDriverState *bs);
> int qbdrvs_img_create(const char *filename, const char *fmt,
>                     const char *base_filename, const char *base_fmt,
>                     char *options, uint64_t img_size, int flags);

s/img_create/create/

Can this return an actual BlockDriverState struct too.

> 
> 
> /* sync access */
> int qbdrvs_read(BlockDriverState *bs, int64_t sector_num,
>               uint8_t *buf, int nb_sectors);
> int qbdrvs_write(BlockDriverState *bs, int64_t sector_num,
>                const uint8_t *buf, int nb_sectors);
> 
> 
> /* info retrieve */
> //sector, size and geometry info
> int qbdrvs_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);

What is in BlockDriverInfo and what is the intended ABI stability
policy for it ?

> int64_t qbdrvs_getlength(BlockDriverState *bs);
> int64_t qbdrvs_get_allocated_file_size(BlockDriverState *bs);
> void qbdrvs_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);

Could this data all just be part of BlockDriverInfo data ?

> //image type
> const char *qbdrvs_get_format_name(BlockDriverState *bs);
> //backing file info
> void qbdrvs_get_backing_filename(BlockDriverState *bs,
>                                char *filename, int filename_size);

You need to include the backing file format here too.

> void qbdrvs_get_full_backing_filename(BlockDriverState *bs,
>                                     char *dest, size_t sz);

Not sure I see why we need this in addition to the above ?


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|