Re: [Qemu-devel] [PATCH 2/2] [WIP]Added target to build libvdisk

[Blue Swirl]

On Sun, Oct 30, 2011 at 16:51, Saggi Mizrahi <address@hidden> wrote:
> RFC: This is my suggestion for wrapping the block layer with another
> supported API. I added only the bare bones verbs I think we can all agree
> on.

It can be expected that the API will only grow: file descriptor
versions, async APIs, thread locks, compat versions etc.

> Apart from the API please also comment on the Makefile. I kinda stitched
> it up and I will appreciate comments on it as well.
>
> libvdisk is a library that packages qemu's handling of disk images. This
> allows for other programs to link to it and get access to qemu image
> file abstractions.

So far, we haven't accepted to export any internals as a library
because of the API and ABI maintenance burdens. Has anything changed?

The licenses of the various files vary, I wonder what conditions
should the shared library have.

> To use install the lib and #include <vdisk.h>
>
> Signed-off-by: Saggi Mizrahi <address@hidden>
> ---
>  .gitignore        |    3 +-
>  Makefile.objs     |    9 ++++
>  libvdisk/Makefile |   53 +++++++++++++++++++++
>  libvdisk/vdisk.c  |  134 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libvdisk/vdisk.h  |   27 +++++++++++
>  5 files changed, 224 insertions(+), 2 deletions(-)
>  create mode 100644 libvdisk/Makefile
>  create mode 100644 libvdisk/vdisk.c
>  create mode 100644 libvdisk/vdisk.h
>
> diff --git a/.gitignore b/.gitignore
> index 6d2acab..7221431 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,5 +1,4 @@
> -config-devices.*
> -config-all-devices.*
> +config-devices.* config-all-devices.*
>  config-host.*
>  config-target.*
>  trace.h
> diff --git a/Makefile.objs b/Makefile.objs
> index 01587c8..a355061 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -171,6 +171,15 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o 
> xen_devconfig.o
>  common-obj-$(CONFIG_XEN_BACKEND) += xen_console.o xenfb.o xen_disk.o 
> xen_nic.o
>
>  ######################################################################
> +# libvdisk
> +
> +vdisk-obj-y = $(block-obj-y)
> +
> +vdisk-obj-y += qemu-tool.o qemu-error.o vdisk.o
> +vdisk-obj-y += $(oslib-obj-y) $(trace-obj-y) $(block-obj-y)
> +vdisk-obj-y += $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
> +
> +######################################################################
>  # libuser
>
>  user-obj-y =
> diff --git a/libvdisk/Makefile b/libvdisk/Makefile
> new file mode 100644
> index 0000000..6b0a28f
> --- /dev/null
> +++ b/libvdisk/Makefile
> @@ -0,0 +1,53 @@
> +# Makefile for libvdisk.
> +
> +include ../config-host.mak
> +include $(SRC_PATH)/rules.mak
> +BUILD_DIR=$(CURDIR)
> +
> +.PHONY: all libvdisk block trace install uninstall
> +
> +$(call set-vpath, $(SRC_PATH))
> +
> +QEMU_CFLAGS+=-I.. -fPIC -I$(BUILD_DIR)
> +
> +include $(SRC_PATH)/Makefile.objs
> +
> +libvdisk: block block.o libvdisk.so.1.0

A variable should be defined for 1.0 and used here and elsewhere.

> +
> +$(BUILD_DIR)/config-host.mak: ../config-host.mak
> +       ln -sf $< $@
> +
> +# Copy because I build the obj with -fPIC which is not needed
> +# for regular qemu build
> +block: trace
> +       mkdir -p block
> +       ln -sf $(SRC_PATH)/block/*.[ch] block
> +trace: trace.h
> +       mkdir -p trace
> +       ln -sf $(SRC_PATH)/trace/*.[chd] trace
> +
> +libvdisk.so.1.0: $(vdisk-obj-y)
> +       $(LD) -shared -soname $@ -o $@ -lc $^
> +
> +install: libvdisk
> +       $(INSTALL) vdisk.h $(PREFIX)$(includedir)
> +       $(INSTALL) libvdisk.so.1.0 $(PREFIX)$(libdir)
> +       ln -sf $(PREFIX)$(libdir)/libvdisk.so.1.0 
> $(PREFIX)$(libdir)/libvdisk.so.1
> +       ln -sf $(PREFIX)$(libdir)/libvdisk.so.1.0 
> $(PREFIX)$(libdir)/libvdisk.so
> +
> +uninstall:
> +       rm -rf $(PREFIX)$(includedir)/vdisk
> +       rm -f $(PREFIX)$(libdir)/libvdisk.so.*
> +
> +all: libvdisk
> +
> +clean:
> +       rm -rf block/
> +       rm -rf trace/
> +       rm -f *.[od]
> +       rm -f *.mak
> +       rm -f trace*
> +       rm -f libvdisk.so.*
> +
> +# Include automatically generated dependency files
> +-include $(wildcard *.d */*.d)
> diff --git a/libvdisk/vdisk.c b/libvdisk/vdisk.c
> new file mode 100644
> index 0000000..d28862d
> --- /dev/null
> +++ b/libvdisk/vdisk.c
> @@ -0,0 +1,134 @@

There is no header, please specify the license.

> +#include "vdisk.h"
> +
> +#include <pthread.h>
> +#include "block.h"
> +
> +typedef struct t_virtual_disk {
> +    BlockDriverState *bs;
> +    pthread_mutex_t mtx;
> +
> +} VirtualDisk;
> +
> +void vdisk_init(void) {
> +    bdrv_init();
> +}
> +
> +static int open_bdrv_from_path(const char* path, BlockDriverState** bs,

Asterisks should reside close to the variable name, not the type.

> +        int flags, const char* format) {
> +    int rc;
> +    BlockDriver* driver;
> +
> +    *bs = bdrv_new(path);
> +    if (format == NULL) {
> +        driver = NULL;
> +    } else {
> +        driver = bdrv_find_format(format);
> +        if (driver == NULL) {
> +            return -EINVAL;
> +        }
> +    }
> +
> +    rc = bdrv_open(*bs, path, flags, driver);
> +    if (rc < 0) {
> +        bdrv_delete(*bs);
> +    }
> +    return rc;
> +}
> +
> +int vdisk_create(const char *filename, const char *fmt,
> +                 const char *base_filename, const char *base_fmt,
> +                 char *options, uint64_t img_size, int flags) {
> +
> +    return bdrv_img_create(filename, fmt, base_filename, base_fmt,
> +            options, img_size, flags);
> +}
> +
> +int vdisk_open(VDD* vdd, const char* pathname, int flags, const char* 
> format) {
> +    int rc;
> +    VirtualDisk* vd = malloc(sizeof(VirtualDisk));
> +    if (!vd) { return -ENOMEM; }
> +
> +    rc = open_bdrv_from_path(pathname, &vd->bs, flags, format);
> +    if (rc < 0) {
> +        goto free_vd;
> +    }
> +
> +    rc = pthread_mutex_init(&vd->mtx, NULL);

I wonder also what thread API should be used, qemu_thread versions
used internally or standard ones. It would be logical to use
qemu_thread versions but then the external user should use those too.

> +    if (rc != 0) {
> +        rc = -rc;
> +        goto free_bs;
> +    }
> +
> +    vdd->virtual_disk = vd;
> +
> +    return 0;
> +
> +free_bs:
> +    bdrv_delete(vd->bs);
> +free_vd:
> +    free(vd);
> +    return rc;
> +}
> +
> +ssize_t vdisk_pread(VDD* vdd, void *buf, size_t count, off_t offset) {
> +    int rc;
> +    uint64_t totalSectors;
> +    VirtualDisk* vd = vdd->virtual_disk;
> +    if (!vd) {
> +        return -EBADF;
> +    }
> +
> +    pthread_mutex_lock(&vd->mtx);
> +    bdrv_get_geometry(vd->bs, &totalSectors);
> +    if ((totalSectors * 512) <= offset) {
> +        rc = 0;
> +        goto end;
> +    }
> +
> +    rc = bdrv_pread(vd->bs, offset, buf, count);
> +end:
> +    pthread_mutex_unlock(&vd->mtx);
> +    return rc;
> +}
> +
> +ssize_t vdisk_pwrite(VDD* vdd, const void *buf, size_t count, off_t offset) {
> +    int rc;
> +    uint64_t totalSectors;
> +    VirtualDisk* vd = vdd->virtual_disk;
> +    if (!vd) {
> +        return -EBADF;
> +    }
> +
> +    pthread_mutex_lock(&vd->mtx);
> +    bdrv_get_geometry(vd->bs, &totalSectors);
> +    if ((totalSectors * 512) <= offset) {
> +        rc = 0;
> +        goto end;
> +    }
> +
> +    rc = bdrv_pwrite(vd->bs, offset, buf, count);
> +end:
> +    pthread_mutex_unlock(&vd->mtx);
> +    return rc;
> +}
> +
> +int vdisk_flush(VDD* vdd) {
> +    VirtualDisk* vd = vdd->virtual_disk;
> +    if (!vd) {
> +        return -EBADF;
> +    }
> +
> +    return bdrv_flush(vd->bs);
> +}
> +
> +int vdisk_close(VDD* vdd) {
> +    VirtualDisk* vd = vdd->virtual_disk;
> +    if (!vd) {
> +        return -EBADF;
> +    }
> +
> +    bdrv_flush(vd->bs);
> +    bdrv_delete(vd->bs);
> +    memset(vdd, 0, sizeof(VDD));
> +    return 0;
> +}
> diff --git a/libvdisk/vdisk.h b/libvdisk/vdisk.h
> new file mode 100644
> index 0000000..6dba7a3
> --- /dev/null
> +++ b/libvdisk/vdisk.h
> @@ -0,0 +1,27 @@

License header missing.

> +#include <unistd.h>
> +#include <inttypes.h>
> +
> +#define VDISK_O_RDWR        0x0002
> +#define VDISK_O_SNAPSHOT    0x0008 /* open the file read only and save 
> writes in a snapshot */
> +#define VDISK_O_NOCACHE     0x0020 /* do not use the host page cache */
> +#define VDISK_O_CACHE_WB    0x0040 /* use write-back caching */
> +#define VDISK_O_NATIVE_AIO  0x0080 /* use native AIO instead of the thread 
> pool */
> +#define VDISK_O_NO_BACKING  0x0100 /* don't open the backing file */
> +#define VDISK_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
> +
> +#define DEFAULT_MAX_DESCIPTORS 1000

DESCRIPTORS, but actually it is unused.

> +
> +typedef struct t_virtual_disk_descriptor {

t_virtual_disk_descriptor should be VDD.

> +    void* virtual_disk;
> +} VDD;
> +
> +void vdisk_init(void);
> +
> +int vdisk_create(const char *filename, const char *fmt,
> +                 const char *base_filename, const char *base_fmt,
> +                 char *options, uint64_t img_size, int flags);
> +int vdisk_open(VDD* vdd, const char* pathname, int flags, const char* 
> format);
> +ssize_t vdisk_pread(VDD* vdd, void *buf, size_t count, off_t offset);
> +ssize_t vdisk_pwrite(VDD* vdd, const void *buf, size_t count, off_t offset);
> +int vdisk_flush(VDD* vdd);
> +int vdisk_close(VDD* vdd);
> --
> 1.7.7
>
>
>