On Fri, Feb 01, 2013 at 01:51:21PM +0800, Wenchao Xia wrote: > >>+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.
Good to hear. > >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. No, it's not okay: QBlockFormatOptionsCOW opts = old_info.o_cow; QBlockFormatInfo new_info; new_info.o_cow = opts; /* broken, only copies old fields */ http://davidz25.blogspot.de/2011/07/writing-c-library-part-5.html#abi-api-versioning http://plan99.net/~mike/writing-shared-libraries.html If you want to allow backwards-compatible changes to QBlockFormatOptionsCOW in the future, then it needs to include padding. > >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. Hmm...I still don't understand the relationship between QBlockImage and a context. Is it safe to use a QBlockImage with context B if it was created with context A? This should be documented. It would be simpler if a QBlockImage is associated with a context. Then you can drop the context argument from functions that operate on a QBlockImage. If necessary for multi-threading, you can provide a function later that associated a QBlockImage with a new context. This allows applications to use QBlockImage with more than one context. > >>+/** > >>+ * 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. We need to be very disciplined about struct definitions that are exposed to applications. There are no compiler warnings when an application copies a libqblock struct, so we cannot prevent (bad) applications from breaking when the struct changes. Either provide the struct definition with padding and allow the application to allocate/free it - then you don't need these helper functions. Or hide the struct and manage allocation/freeing and field access with accessor functions. If you try to mix these approaches the ABI will break when the library changes. > >>+/* 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. Kevin: Does the block layer do short reads/writes? > >>+/** > >>+ * 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? Always use the strings ("qcow2", "raw", etc). strcmp() on a 4 or 5-byte string is very cheap and eliminates the need to convert between strings and enums. Dropping the enum also means one less thing to update when a new image format is added. Stefan