[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Lorenzo Campedelli]

Johannes Schindelin wrote:
> Hi,
>
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
>
> > I've been having problems using vvfat virtual block device. Even linux 
> > fdisk was able to find problems with it. The reason turned out to be 
> > simple, MBR have bogus parameters.
>
> Thanks for doing this; I did not find any time for that.
>
> Overall, I like what you did, but here are some comments (if you would 
> have inlined the patch, I would have commented with references):
>
> - I like the convert_sector2CHS() function, although I would have named it 
>   sector2CHS() for brevity (although the pretty magic -- or unintuitive 
>   -- detection if lba is needed would have to be done differently, which 
>   I maintain would be better),
>
> - you write the NT-ID byte-per-byte, whereas I would have used strcpy() 
>   for clarity,
>
> - I'd have introduced a member nt_id instead of hardcoding an offset into 
>   the "ignored" part, and
>
> - fat_type == 12 and lba does not make sense, or does it?
>
> Thanks,
> Dscho

While you are working on vvfat issues, could you give a look to the 
attached small patch?

It tries to fix the problem with using vvfat together with -snapshot.

The patch is not complete (it doesn't fix additional options such as 
:rw:, :floppy: :32: etc,) nor clean (I guess this specific code should 
be in block-vvfat.c, not in block.c, but the backing file handling is 
done there...) but I needed a quick fix for my needs.

If anybody has suggestions on how to make a better patch to be 
integrated in CVS I'd be glad :)

While I'm here I'd add a related question: wouldn't it be useful to be 
able to specify a per-device "-snapshot" option? Is it doable?

Thanks,
Lorenzo

--- qemu-0.9.0.20070921/block.c.orig    2007-09-21 21:10:53.000000000 +0200
+++ qemu-0.9.0.20070921/block.c 2007-09-22 10:09:32.000000000 +0200
@@ -331,6 +331,7 @@
 
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
+        BlockDriver *drv1;
         int64_t total_size;
 
         /* if snapshot, we create a temporary backing file and open it
@@ -346,10 +347,22 @@
             return -1;
         }
         total_size = bdrv_getlength(bs1) >> SECTOR_BITS;
+        drv1 = bs1->drv;
         bdrv_delete(bs1);
 
         get_tmp_filename(tmp_filename, sizeof(tmp_filename));
-        realpath(filename, backing_filename);
+        /*
+         * for vvfat protocol the string "fat:<options>:" should remain
+         * the prefix of the filename even after realpath() call ...
+         */
+        if (drv1 == &bdrv_vvfat) {
+            int i = strrchr(filename, ':') - filename + 1;
+
+            strncpy(backing_filename, filename, i);
+            realpath(filename + i, backing_filename + i);
+        } else {
+            realpath(filename, backing_filename);
+        }
         if (bdrv_create(&bdrv_qcow2, tmp_filename,
                         total_size, backing_filename, 0) < 0) {
             return -1;