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

[Saggi Mizrahi <smizrahi@redhat.com>]

On Sun 30 Oct 2011 07:21:13 PM IST, Blue Swirl wrote:
> On Sun, Oct 30, 2011 at 16:51, Saggi Mizrahi<smizrahi@redhat.com>  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.
There is no doubt it's going to grow but I intend this to be a tools 
API rather then a whole library to write qemu alternatives. My desire 
is to expose basic functionality so other tools (like VDSM) can do 
image manipulation\conversion\inspection. It is not intended to be 
highly optimized but rather simple to use. Further more I suggest it as 
a guideline to have it not interfere with qemu's internals so they can 
remain optimized.
>
>> 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?
In VDSM we want to be able to manipulate images and using qemu-img just 
doesn't cut it. I don't intend to export the internal API, rather I 
would like to create an alternative simple API that is easier to 
maintain that can be consumed by 3rd party. I tried to make the library 
independent from internal qemu data structures and low level 
functinality so it can be easily maintained.
>
> The licenses of the various files vary, I wonder what conditions
> should the shared library have.
For VDSM GPL is good enough (VDSM being GPL). If there are other 
parties interested in making it a more relaxed licence I'm sure there 
is way to pull something off. This can always happen down the road. I 
know that libvirt would like it to be LGPL. In any case I will submit 
my files as LGPL.
>
>> To use install the lib and #include<vdisk.h>
>>
>> Signed-off-by: Saggi Mizrahi<smizrahi@redhat.com>
>> ---
>>   .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.
I would prefer not to use qemu's threading as it'll expose internal 
APIs. I suggest glib's threading,
>
>> +    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
>>
>>
>>
Thank you, All comments will be addressed. But I would like to get a 
green light for the whole process before sending another version.