From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrQAY-00081u-2n for qemu-devel@nongnu.org; Wed, 18 Jul 2012 05:03:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SrQAS-00080P-4S for qemu-devel@nongnu.org; Wed, 18 Jul 2012 05:03:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55244) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrQAR-00080B-Se for qemu-devel@nongnu.org; Wed, 18 Jul 2012 05:03:36 -0400 Message-ID: <50067BDB.8000309@redhat.com> Date: Wed, 18 Jul 2012 11:03:23 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <4FFA9C30.2070201@linux.vnet.ibm.com> <4FFAA0C3.3080703@redhat.com> <4FFBB7FB.3070303@linux.vnet.ibm.com> <4FFBD6F1.90403@redhat.com> <20120713091611.GC15503@stefanha-thinkpad.localdomain> <4FFFEF8E.5080705@redhat.com> <50000793.2020401@redhat.com> <5003CDC6.2040103@linux.vnet.ibm.com> <5003CE8B.20804@redhat.com> <500678F7.1030705@linux.vnet.ibm.com> In-Reply-To: <500678F7.1030705@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] introduce a dynamic library to expose qemu block API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: Anthony Liguori , Stefan Hajnoczi , Michael Tokarev , =?UTF-8?B?TGx1w60=?= =?UTF-8?B?cw==?= , qemu-devel@nongnu.org, Blue Swirl , Stefan Weil , Hannes Reinecke Il 18/07/2012 10:51, Wenchao Xia ha scritto: > 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. block_int.h is BSD-licensed. It's compatible. > 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. I'll note that img_create, and almost all retrieval functions cannot be implemented on top of NBD. > /* structure init and uninit */ > BlockDriverState *qbdrvs_new(const char *device_name); device_name is not needed in the library. > void qbdrvs_delete(BlockDriverState *bs); Being able to reuse the same BDS with close/open is one of the worst parts of the QEMU block layer. It is only needed to implement eject in QEMU. Let's get things right, and only have open/close: int qbdrvs_open(BlockDriverState **bs, const char *filename, int flags, const char *format_name); void qbdrvs_close(BlockDriverState *bs); > > > /* file open and close */ > int qbdrvs_open(BlockDriverState *bs, const char *filename, int flags, > BlockDriver *drv); You didn't have an API to find the BlockDriver *. Let's just pass a format name, or NULL for probing (see above). This is consistent with qbdrvs_img_create. > 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); > > > /* 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); I would like to have also a scatter gather API (qbdrvs_readv and qbdrvs_writev) taking a "struct iovec *iov, int niov" instead of "uint8_t *buf, int nb_sectors". flush is missing. > > /* info retrieve */ > //sector, size and geometry info > int qbdrvs_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); One problem here is that BlockDriverInfo may grow in the future. This is a problem for the ABI of the library. I don't have any particular ideas here, except adding a separate API to the library for each member of BlockDriverInfo. > int64_t qbdrvs_getlength(BlockDriverState *bs); qbdrvs_get_length. > int64_t qbdrvs_get_allocated_file_size(BlockDriverState *bs); > void qbdrvs_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr); Not needed, it's the same as bdrv_getlength. > //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); > > > /* 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); > int qbdrvs_has_zero_init(BlockDriverState *bs); Paolo