From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0rso-0005JX-LB for qemu-devel@nongnu.org; Mon, 13 Aug 2012 06:28:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T0rsm-0005Ry-O6 for qemu-devel@nongnu.org; Mon, 13 Aug 2012 06:28:26 -0400 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:39570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0rsl-0005Ra-Td for qemu-devel@nongnu.org; Mon, 13 Aug 2012 06:28:24 -0400 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 13 Aug 2012 20:27:29 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7DAJbVY13303984 for ; Mon, 13 Aug 2012 20:19:38 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7DAS8uV020929 for ; Mon, 13 Aug 2012 20:28:08 +1000 Message-ID: <5028D680.7040108@linux.vnet.ibm.com> Date: Mon, 13 Aug 2012 18:27:12 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1344507137-11173-1-git-send-email-xiawenc@linux.vnet.ibm.com> <5024E6FD.6030507@redhat.com> In-Reply-To: <5024E6FD.6030507@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [qemu-devel] [PATCH V2 2/3] [RFC] libqblock-API design List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Paolo Bonzini , Blue Swirl , Stefan Hajnoczi , Anthony Liguori 于 2012-8-10 18:48, Paolo Bonzini 写道: > Il 09/08/2012 12:12, Wenchao Xia ha scritto: >> +/* copy information and take care the member difference in differect version. >> + Assuming all new member are added in the tail, struct size is the first >> + member, this is old to new version, src have its struct_size set. */ >> +static void qboc_adjust_o2n(struct QBlockOptionCreate *dest, >> + struct QBlockOptionCreate *src) >> +{ >> + /* for simple it does memcpy now, need to take care of embbed structure */ >> + memcpy(dest, src, src->struct_size); > > You need an assertion that src->struct_size < sizeof(*dest). > > However, the structure size perhaps wasn't my brightest idea ever. But > still thanks for preparing this prototype! Now that we have a more > complete API to discuss, we can iterate to something better. > > >> + assert(0 == set_option_parameter_int(param, >> + BLOCK_OPT_SIZE, o_cow->virt_size)); > > Assertions should not have side effects. > what side effects would this line have? > >> diff --git a/libqblock.h b/libqblock.h >> new file mode 100644 >> index 0000000..d2e9502 >> --- /dev/null >> +++ b/libqblock.h >> @@ -0,0 +1,447 @@ >> +/* >> + * Copyright IBM Corp. 2012 >> + * >> + * Authors: >> + * Wenchao Xia >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >> + */ >> + >> +#ifndef LIBQBLOCK_H >> +#define LIBQBLOCK_H >> + >> +#include >> +#include >> +#include >> + >> +#define bool _Bool >> + >> +#define QB_ERR_MEM_ERR (-1) >> +#define QB_ERR_INTERNAL_ERR (-2) >> +#define QB_ERR_INVALID_PARAM (-3) >> + >> +/* this library is designed around this core struct. */ >> +struct QBlockState; >> + >> +/* >> + libarary init >> + This function get the library ready to use. >> + */ >> +void libqblock_init(void); >> + >> +/* >> + create a new qbs object >> + params: >> + qbs: out, pointer that will receive created obj. >> + return: >> + 0 on succeed, negative on failure. >> + */ >> +int qb_state_new(struct QBlockState **qbs); >> + >> +/* >> + delete a qbs object >> + params: >> + qbs: in, pointer that will be freed. *qbs will be set to NULL. >> + return: >> + void. >> + */ >> +void qb_state_free(struct QBlockState **qbs); >> + >> + >> +/* flag used in open and create */ >> +#define LIBQBLOCK_O_RDWR 0x0002 >> +/* open the file read only and save writes in a snapshot */ >> +#define LIBQBLOCK_O_SNAPSHOT 0x0008 > > I'd rather avoid exposing this for now. > >> +/* do not use the host page cache */ >> +#define LIBQBLOCK_O_NOCACHE 0x0020 >> +/* use write-back caching */ >> +#define LIBQBLOCK_O_CACHE_WB 0x0040 >> +/* use native AIO instead of the thread pool */ >> +#define LIBQBLOCK_O_NATIVE_AIO 0x0080 > > NATIVE_AIO should be an option for the file protocol. But it's mostly > for performance and since we only support synchronous I/O we can drop it > for now. > > >> +/* don't open the backing file */ >> +#define LIBQBLOCK_O_NO_BACKING 0x0100 >> +/* disable flushing on this disk */ >> +#define LIBQBLOCK_O_NO_FLUSH 0x0200 >> +/* copy read backing sectors into image */ >> +#define LIBQBLOCK_O_COPY_ON_READ 0x0400 > > I'd rather avoid exposing this for now too. > >> +/* consistency hint for incoming migration */ >> +#define LIBQBLOCK_O_INCOMING 0x0800 > > This is internal to QEMU. > > Please add a mask of valid bits and an assertion that unknown bits are zero. > OK to hide this flag now. Do you think this flag belongs to the control flag or I should sort them to anther option? In long term the library should provide full capablities as qemu block layer have so qemu can try migrate to it, so now I need to make sure the API designed have left room for all features qemu have support. >> + >> +#define LIBQBLOCK_O_CACHE_MASK \ >> + (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH) >> + >> +enum QBlockProtocol { >> + QB_PROTO_FILE = 0, >> + QB_PROTO_MAX >> +}; >> + >> +enum QBlockFormat { >> + QB_FMT_NONE = 0, >> + QB_FMT_COW, >> + QB_FMT_QED, >> + QB_FMT_QCOW, >> + QB_FMT_QCOW2, >> + QB_FMT_RAW, >> + QB_FMT_RBD, >> + QB_FMT_SHEEPDOG, >> + QB_FMT_VDI, >> + QB_FMT_VMDK, >> + QB_FMT_VPC, >> + QB_FMT_MAX >> +}; >> + >> +/* block target location info, it include all information about how to find >> + the image */ >> +struct QBlockLocInfo { >> + int struct_size; >> + const char *filename; >> + enum QBlockProtocol protocol; >> +}; >> + >> +/* how to open the image */ >> +struct QBlockOptionOpen { >> + int struct_size; >> + struct QBlockLocInfo o_loc; /* how to find */ >> + enum QBlockFormat o_fmt_type; /* how to extract */ >> + int o_flag; /* how to control */ >> +}; > > You are right that the embedded structs are very complicated. I think > we need to find the right balance between structs for extensibility and > function arguments for ease of use. > > For example, we could have > > int qb_open(struct QBlockState *qbs, > struct QBlockLocation *loc, > struct QBlockFormatOption *op, > int o_flag); > > where: > > - QBlockLocation is basically your QBlockLocInfo, but restructured to > use unions for the protocol-specific fields. > > - QBlockFormatOption is basically your QBlockOptionFormat struct. Thus > we can plan for specifying format-specific options at open time rather > than just at create time (this can be useful, for example, for things > such as lazy_refcounts). Until support is there in the block layer, we > can simply check that all format-specific options are zero. > > Since both QBlockLocation and QBlockFormatOption are basically a > discriminated record (enum + union of structs) we can add a large char > member to the union (e.g. char padding[512]) to keep the ABI stable. > Users must zero out all fields, and future additions must ensure that > the default value is represented by all-zeros. > >> + const char *prealloc_mode; /* off or metadata */ > > Make this an enum. > >> + bool prealloc_mode; > > Here too. > >> +}; >> + >> +struct QBlockOption_vmdk { >> + int struct_size; >> + size_t virt_size; >> + const char *backing_file; >> + int backing_flag; >> + bool compat_version6; >> + const char *subfmt; >> + /* vmdk flat extent format, values: >> + "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | >> + twoGbMaxExtentFlat | streamOptimized} */ > > Here too. > >> +/* information */ >> +/* image related info, static information, from user perspective. */ >> +/* now it is a plain structure, wonder if it could be foldered into embbed one >> + to reflect that format related information better. */ >> +struct QBlockInfoImage { > > QBlockImageInfo > >> + int struct_size; > > Here the struct size makes more sense, because it's a returned struct. > >> + char *filename; >> + enum QBlockProtocol protocol; > > Can this just be a struct QBlockLocation * (using a pointer avoids > problems with embedded structs)? > >> + enum QBlockFormat format; > > Can this just be a struct QBlockFormatOption * (same as the above)? > Yes, I tried it before but found that structure alignment is hard to solve, but as you suggest, I think plain structure plus pointer could make it simpler. Blue Swirl suggest using a API to unify property setting and solve version difference, not using structure, as: qb_set_property(s, "filename", "c:\\\\autoexec.bat"); What do you think of that way? >> + size_t virt_size; >> + /* advance info */ >> + size_t allocated_size; >> + bool encrypt; >> + char *backing_filename; >> +}; >> + >> +/* image info */ >> +/* >> + get image info. >> + params: >> + qbs: in, pointer to QBlockState. >> + info, out, pointer that would receive the information. >> + return: >> + negative on fail, 0 on success. >> + */ >> +int qb_infoimage_get(struct QBlockState *qbs, struct QBlockInfoImage **info); > > qb_get_image_info > OK. >> + >> +/* >> + free image info. >> + params: >> + info, in, pointer, *info would be set to NULL after function called. >> + return: >> + void. >> + */ >> +void qb_infoimage_free(struct QBlockInfoImage **info); > > qb_free_image_info > > Paolo > >> + >> +/* misc */ >> +bool qb_supports_format(enum QBlockFormat fmt); >> +bool qb_supports_protocol(enum QBlockProtocol proto); >> + >> +const char *qb_error_get_detail(struct QBlockState *qbs, int *err_num); >> +#endif >> > > -- Best Regards Wenchao Xia