From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U19Xu-0001o2-2u for qemu-devel@nongnu.org; Fri, 01 Feb 2013 00:52:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U19Xr-0007Em-Bv for qemu-devel@nongnu.org; Fri, 01 Feb 2013 00:52:18 -0500 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:32885) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U19Xq-0007EB-9H for qemu-devel@nongnu.org; Fri, 01 Feb 2013 00:52:15 -0500 Received: from /spool/local by e23smtp01.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 1 Feb 2013 15:47:00 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 99D37357804E for ; Fri, 1 Feb 2013 16:52:05 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r115dxcl10355078 for ; Fri, 1 Feb 2013 16:39:59 +1100 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 r115q4JY003812 for ; Fri, 1 Feb 2013 16:52:04 +1100 Message-ID: <510B57D9.8000609@linux.vnet.ibm.com> Date: Fri, 01 Feb 2013 13:51:21 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1359622430-3936-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1359622430-3936-8-git-send-email-xiawenc@linux.vnet.ibm.com> <20130131154900.GC27038@stefanha-thinkpad.redhat.com> In-Reply-To: <20130131154900.GC27038@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org Thank u for reviewing so many code. >> + >> +#ifndef LIBQBLOCK_ERROR >> +#define LIBQBLOCK_ERROR >> + >> +#include "libqblock-types.h" >> + >> +#define QB_ERR_INTERNAL_ERR (-1) >> +#define QB_ERR_FATAL_ERR (-2) >> +#define QB_ERR_INVALID_PARAM (-100) >> +#define QB_ERR_BLOCK_OUT_OF_RANGE (-101) >> + >> +/* error handling */ >> +/** >> + * qb_error_get_human_str: get human readable error string. >> + * >> + * return a human readable string, it would be truncated if buf is not big >> + * enough. >> + * >> + * @context: operation context, must be valid. >> + * @buf: buf to receive the string. >> + * @buf_size: the size of the string buf. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_error_get_human_str(QBlockContext *context, >> + char *buf, size_t buf_size); > > This function is broken. There is no way to find out how big buf must > be and there is no way to find if the result is truncated or not. > > This is actually worse than: > #define QB_ERROR_HUMAN_SIZE 512 > /* buf must be QB_ERROR_HUMAN_SIZE */ > void qb_error_get_human_str(QBlockContext *context, char *buf); > > At least here the caller knows they will get the full error message > because the library designer knows the error message lengths better than > the application writer. > > The nicest would be: > > /* Return human-readable error message. The caller must use free(3). */ > char *qb_error_get_human_str(QBlockContext *context); > OK, I'll use this format. >> + >> +/** >> + * qb_error_get_errno: get error number, only valid when err_ret is >> + * QB_ERR_INTERNAL_ERR. >> + * >> + * return negative errno if last error is QB_ERR_INTERNAL_ERR, otherwise 0. >> + * >> + * @context: operation context. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_error_get_errno(QBlockContext *context); > > QB_ERR_INTERNAL_ERR is an internal error which means the caller doesn't > know exactly what happened. How is the application supposed to use the > errno? > > The only thing the application could do is strerror(3) but > qb_error_get_human_str() should already do that. > > Therefore qb_error_get_errno() serves no purpose. > will remove it. >> + >> +#endif >> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h >> new file mode 100644 >> index 0000000..91521f5 >> --- /dev/null >> +++ b/libqblock/libqblock-internal.h >> @@ -0,0 +1,68 @@ >> + >> +#define G_LIBQBLOCK_ERROR g_libqblock_error_quark() >> + >> +static inline GQuark g_libqblock_error_quark(void) >> +{ >> + return g_quark_from_static_string("g-libqblock-error-quark"); >> +} > > qb_error_quark() would be a better name. g_*() is for GLib not for code > using glib. > > The shorter name also means you can drop the macro. Macros like > QB_FREE() and G_LIBQBLOCK_ERROR hide a very simple operation. They > force people reading the code to jump to its definition; it interrupts > the reader and forces them to remember abstractions that add no value. > OK. >> +#endif >> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h >> index e69de29..d49b321 100644 >> --- a/libqblock/libqblock-types.h >> +++ b/libqblock/libqblock-types.h >> @@ -0,0 +1,245 @@ >> +/* >> + * QEMU block layer library >> + * >> + * Copyright IBM, Corp. 2013 >> + * >> + * Authors: >> + * Wenchao Xia >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +#ifndef LIBQBLOCK_TYPES_H >> +#define LIBQBLOCK_TYPES_H >> + >> +#include >> +#include >> +#include >> + >> +/* >> + * In case libqblock is used by other project which have not defined >> + * QEMU_DLL_PUBLIC. >> + */ >> +#ifndef QEMU_DLL_PUBLIC >> +#define QEMU_DLL_PUBLIC >> +#endif >> + >> +/* this library is designed around this core struct. */ >> +typedef struct QBlockImage QBlockImage; >> + >> +/* every thread should have a context. */ >> +typedef struct QBlockContext QBlockContext; >> + >> +/* flag used in open and create */ >> +#define LIBQBLOCK_O_RDWR 0x0002 >> +/* do not use the host page cache */ >> +#define LIBQBLOCK_O_NOCACHE 0x0020 >> +/* use write-back caching */ >> +#define LIBQBLOCK_O_CACHE_WB 0x0040 >> +/* don't open the backing file */ >> +#define LIBQBLOCK_O_NO_BACKING 0x0100 >> +/* disable flushing on this disk */ >> +#define LIBQBLOCK_O_NO_FLUSH 0x0200 >> + >> +#define LIBQBLOCK_O_CACHE_MASK \ >> + (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH) >> + >> +#define LIBQBLOCK_O_VALID_MASK \ >> + (LIBQBLOCK_O_RDWR | LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | \ >> + LIBQBLOCK_O_NO_BACKING | LIBQBLOCK_O_NO_FLUSH) > > It is tempting to simplify the libqblock cache options so that: > > * The host page cache is always used. > * Flush operations are required to ensure writes reach the disk. > > This means disk images would behave like regular files. And it would > prevent a lot of confusion about the meaning of the cache modes. > > However, advanced applications might want to use O_DIRECT or even > NO_FLUSH. So I guess it's okay to expose this. > OK, will add those comments. >> + >> +typedef enum QBlockProtocol { >> + QB_PROTO_NONE = 0, >> + QB_PROTO_FILE, >> + QB_PROTO_MAX >> +} QBlockProtocol; > > What prevents libqblock from supporting all protocols? > I think no problem exist in supporting all protocols, it just need more work to sort out the options in protocols, so removed them in 1st version. >> +typedef struct QBlockProtocolOptionsFile { >> + const char *filename; >> +} QBlockProtocolOptionsFile; >> + >> +/** >> + * struct QBlockLocationInfo: contains information about how to find the image >> + * >> + * @prot_type: protocol type, now only support FILE. >> + * @o_file: file protocol related attributes. >> + * @reserved: reserved bytes for ABI. >> + */ >> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512) >> +typedef struct QBlockLocationInfo { >> + QBlockProtocol prot_type; >> + union { >> + QBlockProtocolOptionsFile o_file; >> + uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE/sizeof(uint64_t)]; >> + }; >> +} QBlockLocationInfo; >> + >> + >> +/* format related options */ >> +typedef enum QBlockFormat { >> + QB_FORMAT_NONE = 0, >> + QB_FORMAT_COW, >> + QB_FORMAT_QED, >> + QB_FORMAT_QCOW, >> + QB_FORMAT_QCOW2, >> + QB_FORMAT_RAW, >> + QB_FORMAT_RBD, >> + QB_FORMAT_SHEEPDOG, >> + QB_FORMAT_VDI, >> + QB_FORMAT_VMDK, >> + QB_FORMAT_VPC, >> + QB_FORMAT_MAX >> +} QBlockFormat; >> + >> +typedef struct QBlockFormatOptionsCOW { >> + QBlockLocationInfo backing_loc; >> +} QBlockFormatOptionsCOW; > > Are these struct definitions frozen? > > An application could do: > > QBlockFormatInfo info = ...; > QBlockFormatOptionsCOW opts = info.o_cow; /* broken */ > > If QBlockFormatOptionsCOW changes size then the application can break. > It's hard to demand that applications don't use these structs directly > and once broken applications rely on it we may have a hard time arguing > with them that it's their fault the application is incompatible with new > versions of libqblock (even if we're technically right). > The size may grow with new member added in tail, and have different cases: 1 user just switched the .so file. It is OK, new added member are ignored. 2 user recompile with new header and .so. It is OK, memory are adjusted. Both seems alright, maybe I missed something? >> +typedef struct QBlockFormatOptionsQCOW2 { >> + QBlockLocationInfo backing_loc; >> + QBlockFormat backing_fmt; >> + bool encrypt; >> + uint64_t cluster_size; /* unit is bytes */ >> + QBlockFormatOptionsQCOW2CompatLv cpt_lv; >> + QBlockFormatOptionsQCOW2PreAlloc pre_mode; >> +} QBlockFormatOptionsQCOW2; > > ...this is an example of a struct where we still need to add fields. > qcow2 is still under development and will get new options. > >> diff --git a/libqblock/libqblock.h b/libqblock/libqblock.h >> index e69de29..d3fcaf5 100644 >> --- a/libqblock/libqblock.h >> +++ b/libqblock/libqblock.h >> @@ -0,0 +1,358 @@ >> +/* >> + * QEMU block layer library >> + * >> + * Copyright IBM, Corp. 2013 >> + * >> + * Authors: >> + * Wenchao Xia >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +/* Note: This library is not thread safe yet. */ >> + >> +#ifndef LIBQBLOCK_H >> +#define LIBQBLOCK_H >> + >> +#include "libqblock-types.h" >> +#include "libqblock-error.h" >> + >> +/** >> + * qb_context_new: allocate a new context. >> + * >> + * Broker is used to pass operation to libqblock, and get feedback from it. > > Broker? > my bad, will fix. >> + * >> + * Returns 0 on success, libqblock negative error value on fail. >> + * >> + * Note this function will try init the library, so it must be called the >> + * first. >> + * >> + * @p_context: used to receive the created struct. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_context_new(QBlockContext **p_context); >> + >> +/** >> + * qb_context_delete: delete context. >> + * >> + * Broker will be freed and set to NULL. >> + * >> + * @p_context: pointer to the struct pointer, *p_context will be set to NULL. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_context_delete(QBlockContext **p_context); >> + >> +/** >> + * qb_image_new: allocate a new QBlockImage struct. >> + * >> + * Subsequent qblock actions will use this struct, ref is set to 1. >> + * >> + * Returns 0 if succeed, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @p_qbi: used to receive the created struct. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_image_new(QBlockContext *context, >> + QBlockImage **p_qbi); >> + >> +/** >> + * qb_image_ref: increase the ref of QBlockImage. >> + * >> + * @context: operation context. >> + * @p_qbi: pointer to the struct's pointer. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_image_ref(QBlockContext *context, >> + QBlockImage **p_qbi); > > Why QBlockImage** instead of QBlockImage*? > OK, will use QBlockImage*. > What is the relationship between QBlockImage and its context? Can I > have two contexts A and B like this: > > qb_image_ref(ctx_a, &qbi); > qb_image_unref(ctx_b, &qbi); > > Is this okay? > it should be OK if block layer is thread safe in the future, a thread should own a context. But caller may need to make sure every context should call ref/unref as pairs. >> + >> +/** >> + * qb_image_unref: decrease the ref of QBlockImage. >> + * >> + * if ref is reduced to 0, p_qbi would be freed and set to NULL, at which time >> + * if image was opened and qb_close was not called before, it would be >> + * automatically closed. >> + * >> + * @context: operation context. >> + * @p_qbi: pointer to the struct's pointer. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_image_unref(QBlockContext *context, >> + QBlockImage **p_qbi); >> + >> +/** >> + * qb_location_info_new: create a new QBlockLocationInfo object. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @p_loc: pointer to receive the new created one. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_location_info_new(QBlockContext *context, >> + QBlockLocationInfo **p_loc); >> + >> +/** >> + * qb_location_info_delete: free a QBlockLocationInfo. >> + * >> + * @context: operation context. >> + * @p_loc: pointer to the object, *p_loc would be set to NULL. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_location_info_delete(QBlockContext *context, >> + QBlockLocationInfo **p_loc); > > Why are these functions necessary? The user has the struct definitions > for QBlockLocationInfo so they can allocate/free them. > More likely a helper function. For ABI libqblock allocate the struct instead of user, as a counter part free is also handered by libqblock, to make sure the new added member is not missed in free. >> + >> +/** >> + * qb_format_info_new: create a new QBlockFormatInfo structure. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @p_fmt: pointer that will receive created struct. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_format_info_new(QBlockContext *context, >> + QBlockFormatInfo **p_fmt); >> + >> +/** >> + * qb_format_info_delete: free QBlockFormatInfo structure. >> + * >> + * @context: operation context. >> + * @p_fmt: pointer to the struct, *p_fmt would be set to NULL. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_format_info_delete(QBlockContext *context, >> + QBlockFormatInfo **p_fmt); > > Same here, I don't see a need for these functions. > >> + >> +/** >> + * qb_open: open a block object. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + * @loc: location options for open, how to find the image. >> + * @fmt: format options, how to extract the data, only valid member now is >> + * fmt->fmt_type, set to NULL if you want to auto discovery the format. >> + * @flag: behavior control flags, it is LIBQBLOCK_O_XXX's combination. >> + * >> + * Note: For raw image, there is a risk that it's content is changed to some >> + * magic value resulting a wrong probing done by libqblock, so don't do >> + * probing on raw images. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_open(QBlockContext *context, >> + QBlockImage *qbi, >> + QBlockLocationInfo *loc, >> + QBlockFormatInfo *fmt, >> + int flag); >> + >> +/** >> + * qb_close: close a block object. >> + * >> + * qb_flush is automatically done inside. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_close(QBlockContext *context, >> + QBlockImage *qbi); >> + >> +/** >> + * qb_create: create a block image or object. >> + * >> + * Note: Create operation would not open the image automatically. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + * @loc: location options for open, how to find the image. >> + * @fmt: format options, how to extract the data. >> + * @flag: behavior control flags, LIBQBLOCK_O_XXX's combination. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_create(QBlockContext *context, >> + QBlockImage *qbi, > > Why is qbi required if the image isn't opened? We should only need > context. > Will remove. >> + QBlockLocationInfo *loc, >> + QBlockFormatInfo *fmt, >> + int flag); > > What is the purpose of LIBQBLOCK_O_* flags if the image is not opened? > It is used for open backing up file, I'll add doc for it. >> + >> + >> +/* sync access */ >> +/** >> + * qb_read: block sync read. >> + * >> + * return number of bytes read, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + * @buf: buffer that receive the content. >> + * @len: length to read. >> + * @offset: offset in the block data. >> + */ >> +QEMU_DLL_PUBLIC >> +int32_t qb_read(QBlockContext *context, >> + QBlockImage *qbi, >> + uint8_t *buf, >> + uint32_t len, >> + uint64_t offset); > > The uint32_t len and int32_t return types don't match up. The return > value needs to be int64_t to handle the full uint32_t range. > > However, the return value is only necessary if we want to do short > reads. How about making the return value int with 0 on success and > negative on error? Don't do short reads. > I think change len to int32_t make more sense, this gives more flexibilty, if we found the implement or API inside may return a not completed operation. >> + >> +/** >> + * qb_write: block sync write. >> + * >> + * return number of bytes written, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + * @buf: buffer that receive the content. >> + * @len: length to write. >> + * @offset: offset in the block data. >> + */ >> +QEMU_DLL_PUBLIC >> +int32_t qb_write(QBlockContext *context, >> + QBlockImage *qbi, >> + const uint8_t *buf, >> + uint32_t len, >> + uint64_t offset); > > Same issue with uint32_t len versus int32_t return types. > >> +/** >> + * qb_flush: block sync flush. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_flush(QBlockContext *context, >> + QBlockImage *qbi); >> + >> + >> +/* advance image APIs */ >> +/** >> + * qb_check_allocation: check if [start, start+lenth-1] was allocated on the > > s/lenth/length/ > OK. >> + * image. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + * @start: start position, unit is byte. >> + * @length: length to check, unit is byte, max is 1TB, otherwise will return >> + * QB_ERR_INVALID_PARAM. >> + * @pstatus: pointer to receive the status, 1 means allocated, >> + * 0 means unallocated. >> + * @plength: pointer to receive the length that all have the same status as >> + * *pstatus. >> + * >> + * Note: after return, start+*plength may have the same status as >> + * start+*plength-1. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_check_allocation(QBlockContext *context, >> + QBlockImage *qbi, >> + uint64_t start, >> + int64_t length, >> + int *pstatus, >> + int64_t *plength); > > Why uint64_t start but int64_t length and *plength? They should all be > unsigned. > bdrv_is_allocated use in with sector unit, so used int64_t with byte unit to simplify it, will change them. >> + >> +/* image information */ >> +/** >> + * qb_get_image_info: get image info. >> + * >> + * return 0 on success, libqblock negative error value on fail. >> + * >> + * @context: operation context. >> + * @qbi: pointer to QBlockImage. >> + * @p_info: pointer that would receive the information. >> + * >> + * *info must be not modified after return, qb_info_image_static_delete will >> + * use the information in it. >> + */ >> +QEMU_DLL_PUBLIC >> +int qb_info_image_static_get(QBlockContext *context, >> + QBlockImage *qbi, >> + QBlockStaticInfo **p_info); >> + >> +/** >> + * qb_delete_image_info: free image info. >> + * >> + * @context: operation context. >> + * @p_info: pointer to the information struct, *p_info will be set to NULL. >> + */ >> +QEMU_DLL_PUBLIC >> +void qb_info_image_static_delete(QBlockContext *context, >> + QBlockStaticInfo **p_info); >> + >> +/* helper functions */ >> +/** >> + * qb_str2fmttype: translate format string to libqblock format enum type. >> + * >> + * return the type, or QB_FORMAT_NONE if string matches none of supported types, >> + * never return invalid value or QB_FORMAT_MAX. >> + * >> + * @fmt: the format string, if NULL it will return QB_FORMAT_NONE. >> + */ >> +QEMU_DLL_PUBLIC >> +QBlockFormat qb_str2fmttype(const char *fmt_str); >> + >> +/** >> + * qb_formattype2str: translate libqblock format enum type to a string. >> + * >> + * return a pointer to the string, or NULL if type is not supported, and >> + * returned pointer must NOT be freed. >> + * >> + * @fmt: the format enum type. >> + */ >> +QEMU_DLL_PUBLIC >> +const char *qb_formattype2str(QBlockFormat fmt_type); > > Why does the QBlockFormat enum exist? Is there an issue with using strings? > These fmt/enum code will always exist in an application to handler different format, the choice is whether libqblock handle it for the application. This is more a choice, my idea do it for user. What is your idea? >> +/** >> + * qb_location_info_dup: duplicate a QBlockLocationInfo instance. >> + * >> + * return a pointer to new allocated one having the same values with input, >> + * it need to be freed by qb_location_info_delete later. Never fail except OOM. >> + * >> + * @loc: pointer to the source instance. >> + */ >> +QEMU_DLL_PUBLIC >> +QBlockLocationInfo *qb_location_info_dup(const QBlockLocationInfo *loc); >> + >> +/** >> + * qb_get_virt_size: get virtual size. >> + * >> + * return a pointer, which pointer to a member in info, or NULL if info is >> + * not valid. >> + * >> + * @info: pointer to the QBlockStaticInfo structure. >> + */ >> +QEMU_DLL_PUBLIC >> +const uint64_t *qb_get_virt_size(const QBlockStaticInfo *info); >> + >> +/** >> + * qb_get_backing_loc: get backing file location. >> + * >> + * return a pointer, which pointer to a member in info, or NULL if info is >> + * not valid, or image have no such property. >> + * >> + * @info: pointer to the QBlockStaticInfo structure. >> + */ >> +QEMU_DLL_PUBLIC >> +const QBlockLocationInfo *qb_get_backing_loc(const QBlockStaticInfo *info); >> + >> +/** >> + * qb_get_encrypt: get encrytion flag. >> + * >> + * return a pointer, which pointer to a member in info, or NULL if info is >> + * not valid, or image have no such property. >> + * >> + * @info: pointer to the QBlockStaticInfo structure. >> + */ >> +QEMU_DLL_PUBLIC >> +const bool *qb_get_encrypt(const QBlockStaticInfo *info); > > These functions suggest the caller shouldn't look inside > QBLockStaticInfo although the struct definition seems to be public? > OK, will hide it. > Stefan > -- Best Regards Wenchao Xia