From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49935) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8X9B-0007Si-Ba for qemu-devel@nongnu.org; Mon, 03 Sep 2012 09:57:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T8X94-0007Fe-R4 for qemu-devel@nongnu.org; Mon, 03 Sep 2012 09:57:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3813) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8X94-0007Fa-I6 for qemu-devel@nongnu.org; Mon, 03 Sep 2012 09:56:54 -0400 Message-ID: <5044B720.5080205@redhat.com> Date: Mon, 03 Sep 2012 07:56:48 -0600 From: Eric Blake MIME-Version: 1.0 References: <1346663926-20188-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1346663926-20188-2-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1346663926-20188-2-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enigA2845AB53391321D78B5960B" Subject: Re: [Qemu-devel] [PATCH 1/6] libqblock APIs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@gmail.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigA2845AB53391321D78B5960B Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 09/03/2012 03:18 AM, Wenchao Xia wrote: > This patch contains the major APIs in the library. > Important APIs: > 1 QBroker. These structure was used to retrieve errors, every thread = must > create one first, Later maybe thread related staff could be added into = it. > 2 QBlockState. It stands for an block image object. > 3 QBlockInfoImageStatic. Now it is not folded with location and forma= t. > 4 ABI was kept with reserved members. >=20 > structure, because it would cause caller more codes to find one member.= Commit message snafu? > +/** > + * libqblock_init: Initialize the library > + */ > +void libqblock_init(void); Is this function safe to call more than once? Even tighter, is it safe to call this function simultaneously from multiple threads? > + > +/** > + * qb_broker_new: allocate a new broker > + * > + * Broker is used to pass operation to libqblock, and got feed back fr= om it. s/got feed back/get feedback/ > + * > + * Returns 0 on success, negative value on fail. Is there any particular interpretation to this negative value, such as negative errno constant, or always -1? > + > +/** > + * qb_state_new: allocate a new QBloctState struct s/Bloct/Block/ > + * > + * Following qblock action were based on this struct Didn't read well. Did you mean: Subsequent qblock actions will use this struct > + * > + * Returns 0 if succeed, negative value on fail. Again, is there any particular meaning to which negative value is returne= d? > + > +/** > + * qb_open: open a block object. > + * > + * return 0 on success, negative on fail. > + * > + * @broker: operation broker. > + * @qbs: pointer to struct QBlockState. > + * @loc: location options for open, how to find the image. > + * @fmt: format options, how to extract the data, only valid member no= w is > + fmt->fmt_type, set NULL if you want auto discovery the format. set to NULL if you want to auto-discover the format Maybe also add a warning about the inherent security risks of attempting format auto-discovery (any raw image must NOT be probed, as the raw image can emulate any other format and cause qemu to chase down chains where it should not). > + * @flag: behavior control flags. What flags are currently defined? > + */ > +int qb_open(struct QBroker *broker, > + struct QBlockState *qbs, > + struct QBlockOptionLoc *loc, > + struct QBlockOptionFormat *fmt, > + int flag); > + > +/** > + * qb_close: close a block object. > + * > + * qb_flush is automaticlly done inside. s/automaticlly/automatically/ > +/** > + * qb_create: create a block image or object. > + * > + * Note: Create operation would not open the image automatically. > + * > + * return negative on fail, 0 on success. > + * > + * @broker: operation broker. > + * @qbs: pointer to struct QBlockState. > + * @loc: location options for open, how to find the image. > + * @fmt: format options, how to extract the data. > + * @flag: behavior control flags. Again, what flags are defined? > + > +/* sync access */ > +/** > + * qb_read: block sync read. > + * > + * return negative on fail, 0 on success. Shouldn't this instead return the number of successfully read bytes, to allow for short reads if offset exceeds end-of-file? If so, should it return ssize_t instead of int? > + * > + * @broker: operation broker. > + * @qbs: pointer to struct QBlockState. > + * @buf: buffer that receive the content. s/receive/receives/ > + * @len: length to read. Is there a magic length for reading the entire file in one go? > + * @offset: offset in the block data. > + */ > +int qb_read(struct QBroker *broker, > + struct QBlockState *qbs, > + const void *buf, > + size_t len, > + off_t offset); You do realize that size_t and off_t are not necessarily the same width; but I'm okay with limiting to size_t reads. > +/** > + * qb_write: block sync write. > + * > + * return negative on fail, 0 on success. Again, this should probably return number of successfully written bytes, as an ssize_t. > + * > + * @broker: operation broker. > + * @qbs: pointer to struct QBlockState. > + * @buf: buffer that receive the content. s/receive/supplies/ > +/* advance image APIs */ > +/** > + * qb_is_allocated: check if following sectors was allocated on the im= age. > + * > + * return negative on fail, 0 or 1 on success. 0 means unallocated, 1 = means > + *allocated. Formatting is off. > + * > + * @broker: operation broker. > + * @qbs: pointer to struct QBlockState. > + * @sector_num: start sector number. If 'sector_num' is beyond the end= of the > + *disk image the return value is 0 and 'pnum' is set to 0. > + * @nb_sectors: how many sector would be checked, it is the max value = 'pnum' > + *should be set to. If nb_sectors goes beyond the end of the disk ima= ge it > + *will be clamped. > + * @pnum: pointer to receive how many sectors are allocated or unalloc= ated. This interface requires the user to know how big a sector is. Would it be any more convenient to the user to pass offsets, rather than sector numbers; and/or have a function for converting between offsets and sector numbers? > + */ > +int qb_is_allocated(struct QBroker *broker, > + struct QBlockState *qbs, > + int64_t sector_num, > + int nb_sectors, Shouldn't nb_sectors be size_t? > + int *pnum); Exactly how does the *pnum argument work? This interface looks like it isn't fully thought out yet. Either I want to know if a chunk of sectors is allocated (I supply start and length of sectors to check), regardless of how many sectors beyond that point are also allocated (pnum makes no sense); or I want to know how many sectors are allocated from a given point (I supply start, and the function returns length, so nb_sectors makes no sense). Either way, I think you are supplying too many parameters for how I envision checking for allocated sectors. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enigA2845AB53391321D78B5960B Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQEcBAEBCAAGBQJQRLcgAAoJEKeha0olJ0NqSz0H/32QGn8AmY5pASs4gMMZ7U15 8I78P3IBP25hXjJ8tQ9BYDfUa/c4m+b/qNzbroHgBUY4T4eCwmcImhK3M0NG2Uwv 92vgN5exbvauHLT6ZfgiKONO/hAt48A89Ouix/vm0qEDac/xm9kSQj5CQxTwp9ON A03uJPs9W79EB9RA6c9J0BaxhfsMUa76doygnT4LWTQrQUrZVxaqgpcGRsHBKjYn TgGR31WWOiP0EvE6juXlPKPjLaj00YxUtjQjghnh290w1JdaSCM0X8UtntFPk0wF bVM4tanM29f+cqS2qYhIiv0nmdXk6jKocA204CxZaRSn5jpR14pje1W+pnbXxtU= =lvw3 -----END PGP SIGNATURE----- --------------enigA2845AB53391321D78B5960B--