* Re: [Qemu-devel] [PATCH V17 07/10] libqblock: libqblock API design and type defines
[not found] ` <20130131154900.GC27038@stefanha-thinkpad.redhat.com>
@ 2013-02-01 5:51 ` Wenchao Xia
2013-02-01 9:32 ` Stefan Hajnoczi
0 siblings, 1 reply; 2+ messages in thread
From: Wenchao Xia @ 2013-02-01 5:51 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, aliguori, qemu-devel
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 <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * 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 <sys/types.h>
>> +#include <stdint.h>
>> +#include <stdbool.h>
>> +
>> +/*
>> + * 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 <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * 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
^ permalink raw reply [flat|nested] 2+ messages in thread