[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and t
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines |
Date: |
Fri, 1 Feb 2013 10:32:32 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Wenchao Xia, 2013/02/01
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Paolo Bonzini, 2013/02/05
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Stefan Hajnoczi, 2013/02/05
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Paolo Bonzini, 2013/02/05
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Wenchao Xia, 2013/02/05
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Stefan Hajnoczi, 2013/02/05
- Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines, Paolo Bonzini, 2013/02/05