qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V4 2/5] libqblock type defines


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V4 2/5] libqblock type defines
Date: Thu, 27 Sep 2012 17:52:40 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120907 Thunderbird/15.0.1

  I will change the code for most of comments, but have some response
below.
Il 27/09/2012 04:23, Wenchao Xia ha scritto:
   This patch contains type and macro defines used in APIs, one file for public
usage by user, one for libqblock internal usage.

Signed-off-by: Wenchao Xia <address@hidden>
---
  libqblock/libqblock-internal.h |   57 +++++++++
  libqblock/libqblock-types.h    |  268 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 325 insertions(+), 0 deletions(-)
  create mode 100644 libqblock/libqblock-internal.h
  create mode 100644 libqblock/libqblock-types.h

diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h
new file mode 100644
index 0000000..d3e983c
--- /dev/null
+++ b/libqblock/libqblock-internal.h
@@ -0,0 +1,57 @@
+/*
+ * QEMU block layer library
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Wenchao Xia   <address@hidden>
+ *
+ * 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_INTERNAL
+#define LIBQBLOCK_INTERNAL
+
+#include "glib.h"
+
+#include "block.h"
+#include "block_int.h"
+#include "libqblock-types.h"
+
+/* this file contains defines and types used inside the library. */
+
+#define FUNC_FREE(p) g_free((p))
+#define FUNC_MALLOC(size) g_malloc((size))
+#define FUNC_CALLOC(nmemb, size) g_malloc0((nmemb)*(size))
+#define FUNC_STRDUP(p) g_strdup((p))
+
+#define CLEAN_FREE(p) { \
+        FUNC_FREE(p); \
+        (p) = NULL; \
+}
+
+/* details should be hidden to user */
+struct QBlockState {
+    BlockDriverState *bdrvs;
+    /* internal used file name now, if it is not NULL, it means
+       image was opened.
+    */
+    char *filename;
+};
+
+struct QBlockContext {
+    /* last error */
+    GError *g_error;
+    int err_ret; /* 1st level of error, the libqblock error number */
+    int err_no; /* 2nd level of error, errno what below reports */
+};
+
+#define G_LIBQBLOCK_ERROR g_libqbock_error_quark()
+
+static inline GQuark g_libqbock_error_quark(void)
+{
+    return g_quark_from_static_string("g-libqblock-error-quark");
+}
+#endif
diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h
new file mode 100644
index 0000000..948ab8a
--- /dev/null
+++ b/libqblock/libqblock-types.h
@@ -0,0 +1,268 @@
+/*
+ * QEMU block layer library
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Wenchao Xia   <address@hidden>
+ *
+ * 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>
+
+#if defined(__GNUC__) && __GNUC__ >= 4
+    #ifdef LIBQB_BUILD
+        #define DLL_PUBLIC __attribute__((visibility("default")))
+    #else
+        #define DLL_PUBLIC
+    #endif
+#else
+    #warning : gcc compiler version < 4, symbols can not be hidden.

You don't need to warn.  If LIBQB_BUILD is not defined, DLL_PUBLIC would
be empty anyway.  If LIBQB_BUILD is defined, you know GCC >= 4.0 because
you pass -fvisibility=hidden from the Makefile.

+#endif
+
+/* this library is designed around this core struct. */
+typedef struct QBlockState QBlockState;
+
+/* 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)
+
+typedef enum QBlockProtType {
+    QB_PROT_NONE = 0,
+    QB_PROT_FILE,
+    QB_PROT_MAX
+} QBlockProtType;

Use QBlockProtocol, QB_PROTO_*.

+typedef struct QBlockProtOptionFile {
+    const char *filename;
+} QBlockProtOptionFile;

Use QBlockProtocolOptionsFile

+#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512)
+typedef union QBlockProtOptionsUnion {
+    QBlockProtOptionFile o_file;
+    uint8_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE];
+} QBlockProtOptionsUnion;

Not really options, because they are required.  I would just not name
the union, and embed it in the struct.  Also please change it to

      uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE / 8];

(Sorry for not thinking about this before).  This will fix the alignment
and ensure that future changes to the union do not change the ABI.

  You are right, I forgot alignment issue before.

+/**
+ * struct QBlockProtInfo: contains information about how to find the image
+ *
+ * @prot_type: protocol type, now only support FILE.
+ * @prot_op: protocol related options.
+ */
+typedef struct QBlockProtInfo {
+    QBlockProtType prot_type;
+    QBlockProtOptionsUnion prot_op;
+} QBlockProtInfo;

What about QBlockLocation instead?  Or QBlockLocationInfo.

+
+/* format related options */
+typedef enum QBlockFmtType {
+    QB_FMT_NONE = 0,
+    QB_FMT_COW,
+    QB_FMT_QED,
+    QB_FMT_QCOW,
+    QB_FMT_QCOW2,
+    QB_FMT_RAW,
+    QB_FMT_RBD,
+    QB_FMT_SHEEPDOG,
+    QB_FMT_VDI,
+    QB_FMT_VMDK,
+    QB_FMT_VPC,
+    QB_FMT_MAX
+} QBlockFmtType;

QBlockFormat; similarly remove "Type" from all enums.

+typedef struct QBlockFmtOptionCow {
+    uint64_t virt_size;
+    QBlockProtInfo backing_loc;
+} QBlockFmtOptionCow;

QBlockFormatOptionsCOW.  Similarly use the plural for all other formats.

+typedef struct QBlockFmtOptionQed {
+    uint64_t virt_size;
+    QBlockProtInfo backing_loc;
+    QBlockFmtType backing_fmt;
+    uint64_t cluster_size; /* unit is bytes */
+    uint64_t table_size; /* unit is clusters */
+} QBlockFmtOptionQed;

Similarly use uppercase for QED, QCOW, etc.

+typedef struct QBlockFmtOptionQcow {
+    uint64_t virt_size;
+    QBlockProtInfo backing_loc;
+    bool encrypt;
+} QBlockFmtOptionQcow;
+
+/* "Compatibility level (0.10 or 1.1)" */
+typedef enum QBlockFmtOptionQcow2CptLv {
+    QBO_FMT_QCOW2_CPT_NONE = 0,
+    QBO_FMT_QCOW2_CPT_V010,
+    QBO_FMT_QCOW2_CPT_V110,
+} QBlockFmtOptionQcow2CptLv;

Please use QBO_ instead of QB_ throughout.  Also write COMPAT instead of
CPT, and remove CPT_NONE since 0.10 is the default:

  __NONE is used to indicate whether this property is set or get, so
it is actually have 3 status than just 2: Not set, V010, V110. It
is the same reason that I use __NONE in bool values, especially __NONE could represent it is not got in information retrieving. Maybe I should
rename them to __NOTSET.

typedef enum QBlockFmtOptionQCOW2Compat {
     QB_FMT_QCOW2_COMPAT_V0_10,
     QB_FMT_QCOW2_COMPAT_V1_1,
} QBlockFmtOptionQCOW2Compat

+/* off or metadata */
+typedef enum QBlockFmtOptionQcow2PreAllocType {
+    QBO_FMT_QCOW2_PREALLOC_NONE = 0,
+    QBO_FMT_QCOW2_PREALLOC_OFF,
+    QBO_FMT_QCOW2_PREALLOC_METADATA,
+} QBlockFmtOptionQcow2PreAllocType;

Please remove NONE since OFF is the default.  However, I would use the enum.

+typedef struct QBlockFmtOptionQcow2 {
+    uint64_t virt_size;
+    QBlockProtInfo backing_loc;
+    QBlockFmtType backing_fmt;
+    bool encrypt;
+    uint64_t cluster_size; /* unit is bytes */
+    QBlockFmtOptionQcow2CptLv cpt_lv;
+    QBlockFmtOptionQcow2PreAllocType pre_mode;
+} QBlockFmtOptionQcow2;
+
+typedef struct QBlockFmtOptionRaw {
+    uint64_t virt_size;
+} QBlockFmtOptionRaw;
+
+typedef struct QBlockFmtOptionRbd {
+    uint64_t virt_size;
+    uint64_t cluster_size;
+} QBlockFmtOptionRbd;
+
+/* off or full */
+typedef enum QBlockFmtOptionSheepdogPreAllocType {
+    QBO_FMT_SD_PREALLOC_NONE = 0,
+    QBO_FMT_SD_PREALLOC_OFF,
+    QBO_FMT_SD_PREALLOC_FULL,
+} QBlockFmtOptionSheepdogPreAllocType;

The default is false, so just make it a bool.

+typedef struct QBlockFmtOptionSheepdog {
+    uint64_t virt_size;
+    QBlockProtInfo backing_loc;
+    QBlockFmtOptionSheepdogPreAllocType pre_mode;
+} QBlockFmtOptionSheepdog;
+
+typedef enum QBlockFmtOptionVdiPreAllocType {
+    QBO_FMT_VDI_PREALLOC_NONE = 0,
+    QBO_FMT_VDI_PREALLOC_FALSE,
+    QBO_FMT_VDI_PREALLOC_TRUE,
+} QBlockFmtOptionVdiPreAllocType;

Please remove NONE, and replace FALSE/TRUE with OFF/METADATA (same as
qcow2).

+typedef struct QBlockFmtOptionVdi {
+    uint64_t virt_size;
+    uint64_t cluster_size;
+    QBlockFmtOptionVdiPreAllocType pre_mode;
+} QBlockFmtOptionVdi;
+
+/* whether compact to vmdk verion 6 */
+typedef enum QBlockFmtOptionVmdkCptLv {
+    QBO_FMT_VMDK_CPT_NONE = 0,
+    QBO_FMT_VMDK_CPT_VMDKV6_FALSE,
+    QBO_FMT_VMDK_CPT_VMDKV6_TRUE,
+} QBlockFmtOptionVmdkCptLv;

Here the default is false, so just make it a bool.

+/* vmdk flat extent format, values:
+"{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse |
+twoGbMaxExtentFlat | streamOptimized} */
+typedef enum QBlockFmtOptionVmdkSubfmtType {
+    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE = 0,
+    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE,

You can remove QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE (whose more
appropriate name would be just ...SUBFMT_NONE) since the default is
QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE.

+    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_FLAT,
+    QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_SPARSE,
+    QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_FLAT,
+    QBO_FMT_VMDK_SUBFMT_STREAM_OPTIMIZED,
+} QBlockFmtOptionVmdkSubfmtType;
+
+typedef struct QBlockFmtOptionVmdk {
+    uint64_t virt_size;
+    QBlockProtInfo backing_loc;
+    QBlockFmtOptionVmdkCptLv cpt_lv;
+    QBlockFmtOptionVmdkSubfmtType subfmt;
+} QBlockFmtOptionVmdk;
+
+/* "{dynamic (default) | fixed} " */
+typedef enum QBlockFmtOptionVpcSubfmtType {
+    QBO_FMT_VPC_SUBFMT_NONE = 0,
+    QBO_FMT_VPC_SUBFMT_DYNAMIC,
+    QBO_FMT_VPC_SUBFMT_FIXED,
+} QBlockFmtOptionVpcSubfmtType;

Again, please remove NONE.

+typedef struct QBlockFmtOptionVpc {
+    uint64_t virt_size;
+    QBlockFmtOptionVpcSubfmtType subfmt;
+} QBlockFmtOptionVpc;
+
+#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2)
+typedef union QBlockFmtOptionsUnion {
+    QBlockFmtOptionCow       o_cow;
+    QBlockFmtOptionQed       o_qed;
+    QBlockFmtOptionQcow      o_qcow;
+    QBlockFmtOptionQcow2     o_qcow2;
+    QBlockFmtOptionRaw       o_raw;
+    QBlockFmtOptionRbd       o_rbd;
+    QBlockFmtOptionSheepdog  o_sheepdog;
+    QBlockFmtOptionVdi       o_vdi;
+    QBlockFmtOptionVmdk      o_vmdk;
+    QBlockFmtOptionVpc       o_vpc;
+    uint8_t reserved[QBLOCK_FMT_OPTIONS_UNION_SIZE];

Similarly use uint64_t here, and just embed the union in the struct.

+} QBlockFmtOptionsUnion;
+typedef struct QBlockFmtInfo {
+    QBlockFmtType fmt_type;
+    QBlockFmtOptionsUnion fmt_op;
+} QBlockFmtInfo;

QBlockFormatInfo.

+/**
+ * QBlockStaticInfoAddr: a structure contains a set of pointer.
+ *
+ *    this struct contains a set of pointer pointing to some
+ *  property related to format or protocol. If a property is not available,
+ *  it will be set as NULL. User could use this to get properties directly.
+ *
+ *  @virt_size: virtual size, it is always not NULL.
+ *  @backing_loc: backing file location.
+ *  @encrypt: encryption flag.
+*/
+
+typedef struct QBlockStaticInfoAddr {
+    uint64_t *virt_size;
+    QBlockProtInfo *backing_loc;
+    bool *encrypt;
+} QBlockStaticInfoAddr;

Why the indirection?

  It helps user to get these important members, otherwise
user will need
Switch (fmt) {
  case RAW:
    ....
  case QCOW2:
    ...
}
for every attribute. The indirection address will let user directly
access the members.

+/**
+ * QBlockStaticInfo: information about the block image.
+ *
+ * @member_addr: contains pointer which can be used to access the structure.
+ * @loc: location information.
+ * @fmt: format information.
+ * @sector_size: how many bytes in a sector, it is 512 usually.
+ */
+typedef struct QBlockStaticInfo {
+    QBlockStaticInfoAddr *member_addr;
+    QBlockProtInfo loc;
+    QBlockFmtInfo fmt;
+    int sector_size;
+} QBlockStaticInfo;
+
+
+#endif




--
Best Regards

Wenchao Xia




reply via email to

[Prev in Thread] Current Thread [Next in Thread]