Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines

[Stefan Hajnoczi]

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