Re: [Qemu-devel] [PATCH] support add-cow file format

[Donald]

于Mon 12 Sep 2011 03:59:07 PM CST,Kevin Wolf写到:
> Am 10.09.2011 02:54, schrieb Dong Xu Wang:
> > 于Fri 09 Sep 2011 10:27:26 PM CST,Kevin Wolf写到:
> > > Am 09.09.2011 07:48, schrieb Dong Xu Wang:
> > > > As raw file format does not support backing_file and copy on write feature, so 
> > > > I add COW to it to support backing_file option. I store dirty bitmap in an 
> > > > add-cow file. When executed, it looks like this:
> > > > qemu-img create -f add-cow -o backing_file=ubuntu.img,image_file=test.img 
> > > > test.add-cow
> > > > qemu -drive if=virtio,file=test.add-cow -m 1024 
> > > >
> > > > (test.img is a raw format file; test.add-cow stores bitmap)
> > > >
> > > > Signed-off-by: Dong Xu Wang <address@hidden>
> > >
> > > You should not make any changes to generic code, except maybe add
> > > something to bdrv_get_info(). In particular you shouldn't need to touch
> > > bdrv_open() or bdrv_create() at all.
> > >
> > > The one required change in the approach for this to work is that you
> > > shouldn't view raw+add_cow as a unit, but add_cow should be treated as
> > > something separate that happens to be stacked on a raw file (which is
> > > created separately).
> > >
> > > Then you can do almost everything in block/add-cow.c.
> > >
> > > > ---
> > > >  Makefile.objs   |    1 +
> > > >  block.c         |   83 ++++++++++-
> > > >  block.h         |    2 +
> > > >  block/add-cow.c |  456 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  block_int.h     |    6 +
> > > >  qemu-img.c      |   10 ++
> > > >  6 files changed, 555 insertions(+), 3 deletions(-)
> > > >  create mode 100644 block/add-cow.c
> > > >
> > > > diff --git a/Makefile.objs b/Makefile.objs
> > > > index 26b885b..1402f9f 100644
> > > > --- a/Makefile.objs
> > > > +++ b/Makefile.objs
> > > > @@ -31,6 +31,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > > >  
> > > >  block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
> > > >  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> > > > qcow2-cache.o
> > > > +block-nested-y += add-cow.o
> > > >  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> > > >  block-nested-y += qed-check.o
> > > >  block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> > > > diff --git a/block.c b/block.c
> > > > index a8c789a..c797cfc 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -369,7 +369,7 @@ static int find_image_format(const char *filename, 
> > > > BlockDriver **pdrv)
> > > >  {
> > > >      int ret, score, score_max;
> > > >      BlockDriver *drv1, *drv;
> > > > -    uint8_t buf[2048];
> > > > +    uint8_t buf[4096];
> > > >      BlockDriverState *bs;
> > >
> > > What's the reason for this change?
> > The size of add_cow_header in my code is larger than 2048.
>
> Right, but the magic is in the first 8 bytes, so for probing 2048 bytes
> should be more than enough.
>
> > > > diff --git a/block/add-cow.c b/block/add-cow.c
> > > > new file mode 100644
> > > > index 0000000..f4b67e5
> > > > --- /dev/null
> > > > +++ b/block/add-cow.c
> > > > @@ -0,0 +1,456 @@
> > > > +#include "qemu-common.h"
> > > > +#include "block_int.h"
> > > > +#include "module.h"
> > > > +
> > > > +#define ADD_COW_MAGIC  (((uint64_t)'A' << 56) | ((uint64_t)'D' << 48) | \
> > > > +                        ((uint64_t)'D' << 40) | ((uint64_t)'_' << 32) | \
> > > > +                        ((uint64_t)'C' << 24) | ((uint64_t)'O' << 16) | \
> > > > +                        ((uint64_t)'W' << 8) | 0xFF)
> > > > +#define ADD_COW_VERSION 1
> > > > +
> > > > +struct add_cow_header {
> > > > +    uint64_t magic;
> > > > +    uint32_t version;
> > > > +    char backing_file[1024];
> > > > +    char image_file[1024];
> > > > +    uint64_t size;
> > > > +    uint32_t sectorsize;
> > > > +} add_cow_header;
> > >
> > > QEMU_PACKED
> > Sorry, what does QEMU_PACKED mean?
>
> This is an on-disk structure, so you need to pack the structure.
> Otherwise the compiler would be free to add padding between the fields
> in order to optimise alignment.
>
> struct add_cow_header {
>     ...
> } QEMU_PACKED add_cow_header;
>
> Hm, actually, do you really want to declare a global variable here? Or
> is a typedef missing? Also, coding style requires the struct name to be
> spelled AddCowHeader.
>
> Kevin
Thank you,Kevin. I will consider these advice in the second version.