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

[Wenchao Xia]

于 2012-7-24 2:15, Blue Swirl 写道:
> On Wed, Jul 18, 2012 at 8:51 AM, Wenchao Xia <address@hidden> 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.
> >
> >
> > /* 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);
>
> How are the errors passed?
   Returning negative value should indicate errors. I plan to insert
a qblock-err.h to define all number there, and then a function of
const char *qberr2str(int err) to translate error.

> Alternative version with file descriptor or struct FILE instead of
> filename might become useful but can be added later.
  Agree.
> > 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);
>
> 'const char *options'
>
> > /* 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);
>
> Do we want to use sectors here? How about just raw byte offsets and
> number of bytes? I'd leave any hardware details out and just provide
> file semantics (open/read/write/close). Future QEMU refactorings could
> make supporting HW info inconvenient. If HW details (geometry etc., VM
> state) are needed, there should be a separate API.
  not sure why original API used sector instead of  raw address,
if no special requirement for sector I think raw address is better.

> Vectored I/O might be useful too.
>
> > /* info retrieve */
> > //sector, size and geometry info
> > int qbdrvs_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>
> Currently BlockDriverInfo does not look very useful for outside users,
> with the exception of dirty state. How about accessors instead:
> bool qbdrvs_is_dirty(BlockDriverState *bs);
>
> Also the format (QCOW) is needed so that the user can use backing file
> functions:
> const char *qbdrvs_get_format(BlockDriverState *bs);
  I hope to package the info into a structure, such as
#define QBLOCK_INFO_SIZE (128)
struct QblockInfo {
    int dirty;
    char *format;
    int reserved[RESERVE_SPACE];
};
assert sizeof(QblockInfo) == QBLOCK_INFO_SIZE in every version. This
will make caller's code more clean.

> > 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);
>
> Also this function shouldn't be needed if we don't give HW details
> with this API.
    OK.

> > //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);
> > void qbdrvs_get_full_backing_filename(BlockDriverState *bs,
> >                                      char *dest, size_t sz);
>
> These are specific to QCOW etc., so
> bool qbdrvs_has_backing_files(BlockDriverState *bs)?
  How about using return value to indicate it?

int qbdrvs_get_backing_filename(BlockDriverState *bs, char *filename, 
int filename_size);

> > /* advanced image content access */
> > int qbdrvs_is_allocated(BlockDriverState *bs, int64_t sector_num, int
> > nb_sectors,
> >                        int *pnum);
> > int qbdrvs_discard(BlockDriverState *bs, int64_t sector_num, int
> > nb_sectors);
>
> Again, some files do not have a concept of allocation.
  Maybe return value could indicate it.

> > int qbdrvs_has_zero_init(BlockDriverState *bs);
> >
> >
> > > Il 16/07/2012 10:16, Wenchao Xia ha scritto:
> > > > >
> > > >     Really thanks for the investigation, I paid quite sometime to dig out
> > > > which license is compatible to LGPL, this have sorted it out.
> > > >     The coroutine and structure inside is quite a challenge.
> > >
> > >
> > > Coroutines are really just a small complication in the program flow if
> > > all you support is synchronous access to files (i.e. no HTTP etc.).
> > > Their usage should be completely transparent.
> > >
> > > > What about
> > > > provide the library first in nbd + sync access, and waiting for the
> > > > library employer response? If it is good to use, then replace implement
> > > > code to native qemu block layer code, change code's license, while keep
> > > > API unchanged.
> > >
> > >
> > > You can start by proposing the API.
> > >
> > > Paolo
> >
> >
> > --
> > Best Regards
> >
> > Wenchao Xia


--
Best Regards

Wenchao Xia