From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1Ct3-000209-DF for qemu-devel@nongnu.org; Tue, 14 Aug 2012 04:54:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1Ct0-0000br-Qg for qemu-devel@nongnu.org; Tue, 14 Aug 2012 04:54:05 -0400 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:59893) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1Csz-0000ba-RR for qemu-devel@nongnu.org; Tue, 14 Aug 2012 04:54:02 -0400 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 14 Aug 2012 14:23:58 +0530 Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7E8rZOS25297094 for ; Tue, 14 Aug 2012 14:23:35 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7E8rZGs029089 for ; Tue, 14 Aug 2012 18:53:35 +1000 Message-ID: <502A11CC.404@linux.vnet.ibm.com> Date: Tue, 14 Aug 2012 16:52:28 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1344507050-11054-1-git-send-email-xiawenc@linux.vnet.ibm.com> <5024C096.9040007@linux.vnet.ibm.com> <5028E4B6.5020708@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [qemu-devel] [PATCH V2 0/3] [RFC] libqblock draft code v2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: stefanha@gmail.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, pbonzini@redhat.com 于 2012-8-14 4:00, Blue Swirl 写道: > On Mon, Aug 13, 2012 at 11:27 AM, Wenchao Xia > wrote: >> 于 2012-8-11 20:18, Blue Swirl 写道: >> >>> On Fri, Aug 10, 2012 at 8:04 AM, Wenchao Xia >>> wrote: >>>> >>>> Thanks for your review, sorry I have forgot some fixing you >>>> mentioned before, will correct them this time. >>>> >>>> 于 2012-8-10 1:12, Blue Swirl 写道: >>>> >>>>> On Thu, Aug 9, 2012 at 10:10 AM, Wenchao Xia >>>>> >>>>> wrote: >>>>>> >>>>>> >>>>>> This patch intrudce libqblock API, libqblock-test is used as a test >>>>>> case. >>>>>> >>>>>> V2: >>>>>> Using struct_size and [xxx]_new [xxx]_free to keep ABI. >>>>>> Using embbed structure to class format options in image creating. >>>>>> Format specific options were brought to surface. >>>>>> Image format was changed to enum interger instead of string. >>>>>> Some API were renamed. >>>>>> Internel error with errno was saved and with an API caller can get >>>>>> it. >>>>>> ALL flags used were defined in libqblock.h. >>>>>> >>>>>> Something need discuss: >>>>>> Embbed structure or union could make the model more friendly, but >>>>>> that >>>>>> make ABI more difficult, because we need to check every embbed >>>>>> structure's >>>>>> size and guess compiler's memory arrangement. This means #pragma >>>>>> pack(4) >>>>>> or struct_size, offset_next in every structure. Any better way to solve >>>>>> it? >>>>>> or make every structure a plain one? >>>>> >>>>> >>>>> >>>>> I'd still use accessor functions instead of structure passing, it >>>>> would avoid these problems. >>>>> >>>> Do you mean some function like : >>>> CreateOption_Filename_Set(const char *filename) >>>> CreateOption_Format_Set(const char *filename) >>> >>> >>> Something like this: >>> int qb_create_cow(struct QBlockState *qbs, const char *filename, >>> size_t virt_size, const char *backing_file, int backing_flag); >>> etc. for rest of the formats. >>> >>> Alternatively, we could have more generic versions like you describe, >>> or even more generic still: >>> >>> void qb_set_property(struct QBlockState *qbs, const char *prop_name, >>> const char *prop_value); >>> >>> so the create sequence (ignoring error handling) would be: >>> s = qb_init(); >>> qb_set_property(s, "filename", "c:\\\\autoexec.bat"); >>> qb_set_property(s, "format", "cow"); >>> qb_set_property(s, "virt_size", "10GB"); >>> // use defaults for backing_file and backing_flag >>> qb_create(s); >>> >>> Likewise for info structure: >>> char *qb_get_property(struct QBlockState *qbs, const char *prop_name); >>> >>> foo = qb_get_property(s, "format"); >>> foo = qb_get_property(s, "encrypted"); >>> foo = qb_get_property(s, "num_backing_files"); >>> foo = qb_get_property(s, "virt_size"); >>> >>> This would be helpful for the client to display parameters without >>> much understanding of their contents: >>> char **qb_list_properties(struct QBlockState *qbs); /* returns array >>> of property names available for this file, use get_property to >>> retrieve their contents */ >>> >>> But the clients can't be completely ignorant of all formats available, >>> for example a second UI dialog needs to be added for formats with >>> backing files, otherwise it won't be able to access some files at all. >>> Maybe by adding type descriptions for each property (type for >>> "filename" is "path", for others "string", "bool", "enum" etc). >>> >> Thanks. This seems heavy document is needed for that no structure >> can indicate what options sub format have, user can only get that info >> from returns or documents. I am not sure if this is good, because it >> looks more like a object oriented API that C. > > This approach may be a bit over-engineered, but it may be simpler to > tie to an UI. > > What do you think of the simple version then: > int qb_create_cow(struct QBlockState *qbs, const char *filename, > size_t virt_size, const char *backing_file, int backing_flag); > it is hard to extend more options, if for every format a API is needed, then int qb_create_cow(struct QBlockState *qbs, struct QBlockOptionFmtCow *op) seems better, while keep struct QBlockOptionFmtCow a plain struct without embbed struct(using pointer instead). >> >> >> >>>> >>>> It can solve the issue, with a cost of more small APIs in header >>>> files that user should use. Not sure if there is a good way to make >>>> it more friendly as an object language: >>>> "oc.filename = name;" automatically call CreateOption_Filename_Set, >>>> API CreateOption_Filename_Set is invisible to user. >>>> >>>> >>>> >>>>> Packing can even introduce a new set of problems since we don't >>>>> control the CFLAGS of the client of the library. >>>> >>>> >>>> >>>> indeed, I tried to handle the difference in function qboo_adjust_o2n, >>>> found that structure member alignment is hard to deal. >>>> >>>> >>>>>> AIO is missing, need a prototype. >>>>>> >>>>>> Wenchao Xia (3): >>>>>> adding libqblock >>>>>> libqblock API >>>>>> libqblock test case >>>>>> >>>>>> Makefile | 3 + >>>>>> block.c | 2 +- >>>>>> block.h | 1 + >>>>>> libqblock-test.c | 197 ++++++++++++++++ >>>>>> libqblock.c | 670 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> libqblock.h | 447 ++++++++++++++++++++++++++++++++++++ >>>>>> 6 files changed, 1319 insertions(+), 1 deletions(-) >>>>>> create mode 100644 libqblock-test.c >>>>>> create mode 100644 libqblock.c >>>>>> create mode 100644 libqblock.h >>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Best Regards >>>> >>>> Wenchao Xia >>>> >>> >> >> >> -- >> Best Regards >> >> Wenchao Xia >> > -- Best Regards Wenchao Xia