[PATCH 0/2] vvfat: fix two crashes.

[PATCH 0/2] vvfat: fix two crashes.

[Vladimir Sementsov-Ogievskiy]

Hi!

As reported by Programmingkid, command 

qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none

crashes.

I tested it with qemu-system-x86_64 and it reproduces for me. I even
kept "<path of a host folder>" as is :).

So, here are two fixes.

Vladimir Sementsov-Ogievskiy (2):
  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
  block/vvfat: fix vvfat_child_perm crash

 include/block/block.h | 1 +
 block.c               | 4 ++--
 block/vvfat.c         | 8 +++-----
 3 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.29.2

[PATCH 1/2] block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash

[Vladimir Sementsov-Ogievskiy]

Commit 3ca1f3225727419ba573673b744edac10904276f
"block: BdrvChildClass: add .get_parent_aio_context handler" introduced
new handler and commit 228ca37e12f97788e05bd0c92f89b3e5e4019607
"block: drop ctx argument from bdrv_root_attach_child" made a generic
use of it. But 3ca1f3225727419ba573673b744edac10904276f didn't update
child_vvfat_qcow. Fix that.

Before that fix the command

./build/qemu-system-x86_64 -usb -device usb-storage,drive=fat16 \
  -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none

crashes:

1  bdrv_child_get_parent_aio_context (c=0x559d62426d20)
    at ../block.c:1440
2  bdrv_attach_child_common
    (child_bs=0x559d62468190, child_name=0x559d606f9e3d "write-target",
     child_class=0x559d60c58d20 <child_vvfat_qcow>, child_role=3,
     perm=3, shared_perm=4, opaque=0x559d62445690,
     child=0x7ffc74c2acc8, tran=0x559d6246ddd0, errp=0x7ffc74c2ae60)
    at ../block.c:2795
3  bdrv_attach_child_noperm
    (parent_bs=0x559d62445690, child_bs=0x559d62468190,
     child_name=0x559d606f9e3d "write-target",
     child_class=0x559d60c58d20 <child_vvfat_qcow>, child_role=3,
     child=0x7ffc74c2acc8, tran=0x559d6246ddd0, errp=0x7ffc74c2ae60) at
    ../block.c:2855
4  bdrv_attach_child
    (parent_bs=0x559d62445690, child_bs=0x559d62468190,
     child_name=0x559d606f9e3d "write-target",
     child_class=0x559d60c58d20 <child_vvfat_qcow>, child_role=3,
     errp=0x7ffc74c2ae60) at ../block.c:2953
5  bdrv_open_child
    (filename=0x559d62464b80 "/var/tmp/vl.h3TIS4",
     options=0x559d6246ec20, bdref_key=0x559d606f9e3d "write-target",
     parent=0x559d62445690, child_class=0x559d60c58d20
     <child_vvfat_qcow>, child_role=3, allow_none=false,
     errp=0x7ffc74c2ae60) at ../block.c:3351
6  enable_write_target (bs=0x559d62445690, errp=0x7ffc74c2ae60) at
   ../block/vvfat.c:3176
7  vvfat_open (bs=0x559d62445690, options=0x559d6244adb0, flags=155650,
               errp=0x7ffc74c2ae60) at ../block/vvfat.c:1236
8  bdrv_open_driver (bs=0x559d62445690, drv=0x559d60d4f7e0
                     <bdrv_vvfat>, node_name=0x0,
                     options=0x559d6244adb0, open_flags=155650,
                     errp=0x7ffc74c2af70) at ../block.c:1557
9  bdrv_open_common (bs=0x559d62445690, file=0x0,
                     options=0x559d6244adb0, errp=0x7ffc74c2af70) at
...

(gdb) fr 1
 #1  0x0000559d603ea3bf in bdrv_child_get_parent_aio_context
     (c=0x559d62426d20) at ../block.c:1440
1440        return c->klass->get_parent_aio_context(c);
 (gdb) p c->klass
$1 = (const BdrvChildClass *) 0x559d60c58d20 <child_vvfat_qcow>
 (gdb) p c->klass->get_parent_aio_context
$2 = (AioContext *(*)(BdrvChild *)) 0x0

Fixes: 3ca1f3225727419ba573673b744edac10904276f
Fixes: 228ca37e12f97788e05bd0c92f89b3e5e4019607
Reported-by: Programmingkid <programmingkidx@gmail.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h | 1 +
 block.c               | 4 ++--
 block/vvfat.c         | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 82185965ff..8e707a83b7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -701,6 +701,7 @@ bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,
 bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,
                               GSList **ignore, Error **errp);
 AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);
+AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c);
 
 int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
 int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
diff --git a/block.c b/block.c
index 0dc97281dc..ef13076c4c 100644
--- a/block.c
+++ b/block.c
@@ -1412,7 +1412,7 @@ static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
     return 0;
 }
 
-static AioContext *bdrv_child_cb_get_parent_aio_context(BdrvChild *c)
+AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c)
 {
     BlockDriverState *bs = c->opaque;
 
@@ -1432,7 +1432,7 @@ const BdrvChildClass child_of_bds = {
     .can_set_aio_ctx = bdrv_child_cb_can_set_aio_ctx,
     .set_aio_ctx     = bdrv_child_cb_set_aio_ctx,
     .update_filename = bdrv_child_cb_update_filename,
-    .get_parent_aio_context = bdrv_child_cb_get_parent_aio_context,
+    .get_parent_aio_context = child_of_bds_get_parent_aio_context,
 };
 
 AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c)
diff --git a/block/vvfat.c b/block/vvfat.c
index 54807f82ca..07232a7cfc 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3130,6 +3130,7 @@ static void vvfat_qcow_options(BdrvChildRole role, bool parent_is_format,
 static const BdrvChildClass child_vvfat_qcow = {
     .parent_is_bds      = true,
     .inherit_options    = vvfat_qcow_options,
+    .get_parent_aio_context = child_of_bds_get_parent_aio_context,
 };
 
 static int enable_write_target(BlockDriverState *bs, Error **errp)
-- 
2.29.2

[PATCH 2/2] block/vvfat: fix vvfat_child_perm crash

[Vladimir Sementsov-Ogievskiy]

It's wrong to rely on s->qcow in vvfat_child_perm, as on permission
update during bdrv_open_child() call this field is not set yet.

Still prior to aa5a04c7db27eea6b36de32f241b155f0d9ce34d, it didn't
crash, as bdrv_open_child passed NULL as child to bdrv_child_perm(),
and NULL was equal to NULL in assertion (still, it was bad guarantee
for child being s->qcow, not backing :).

Since aa5a04c7db27eea6b36de32f241b155f0d9ce34d
"add bdrv_attach_child_noperm" bdrv_refresh_perms called on parent node
when attaching child, and new correct child pointer is passed to
.bdrv_child_perm. Still, s->qcow is NULL at the moment. Let's rely only
on role instead.

Without that fix,
./build/qemu-system-x86_64 -usb -device usb-storage,drive=fat16 \
    -drive \
    file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none

crashes:
(gdb) bt
0  raise () at /lib64/libc.so.6
1  abort () at /lib64/libc.so.6
2  _nl_load_domain.cold () at /lib64/libc.so.6
3  annobin_assert.c_end () at /lib64/libc.so.6
4  vvfat_child_perm (bs=0x559186f3d690, c=0x559186f1ed20, role=3,
                     reopen_queue=0x0, perm=0, shared=31,
                     nperm=0x7ffe56f28298, nshared=0x7ffe56f282a0) at
    ../block/vvfat.c:3214
5  bdrv_child_perm (bs=0x559186f3d690, child_bs=0x559186f60190,
                    c=0x559186f1ed20, role=3, reopen_queue=0x0,
                    parent_perm=0, parent_shared=31,
                    nperm=0x7ffe56f28298, nshared=0x7ffe56f282a0)
    at ../block.c:2094
6  bdrv_node_refresh_perm (bs=0x559186f3d690, q=0x0,
                           tran=0x559186f65850, errp=0x7ffe56f28530) at
    ../block.c:2336
7  bdrv_list_refresh_perms (list=0x559186db5b90 = {...}, q=0x0,
                            tran=0x559186f65850, errp=0x7ffe56f28530)
    at ../block.c:2358
8  bdrv_refresh_perms (bs=0x559186f3d690, errp=0x7ffe56f28530) at
    ../block.c:2419
9  bdrv_attach_child
    (parent_bs=0x559186f3d690, child_bs=0x559186f60190,
     child_name=0x559184d83e3d "write-target",
     child_class=0x5591852f3b00 <child_vvfat_qcow>, child_role=3,
     errp=0x7ffe56f28530) at ../block.c:2959
10 bdrv_open_child
    (filename=0x559186f5cb80 "/var/tmp/vl.7WYmFU",
     options=0x559186f66c20, bdref_key=0x559184d83e3d "write-target",
     parent=0x559186f3d690, child_class=0x5591852f3b00
     <child_vvfat_qcow>, child_role=3, allow_none=false,
     errp=0x7ffe56f28530) at ../block.c:3351
11 enable_write_target (bs=0x559186f3d690, errp=0x7ffe56f28530) at
    ../block/vvfat.c:3177
12 vvfat_open (bs=0x559186f3d690, options=0x559186f42db0, flags=155650,
               errp=0x7ffe56f28530) at ../block/vvfat.c:1236
13 bdrv_open_driver (bs=0x559186f3d690, drv=0x5591853d97e0
                     <bdrv_vvfat>, node_name=0x0,
                     options=0x559186f42db0, open_flags=155650,
                     errp=0x7ffe56f28640) at ../block.c:1557
14 bdrv_open_common (bs=0x559186f3d690, file=0x0,
                     options=0x559186f42db0, errp=0x7ffe56f28640) at
    ../block.c:1833
...

(gdb) fr 4
 #4  vvfat_child_perm (bs=0x559186f3d690, c=0x559186f1ed20, role=3,
                      reopen_queue=0x0, perm=0, shared=31,
                      nperm=0x7ffe56f28298, nshared=0x7ffe56f282a0) at
    ../block/vvfat.c:3214
3214        assert(c == s->qcow || (role & BDRV_CHILD_COW));
(gdb) p role
 $1 = 3   # BDRV_CHILD_DATA | BDRV_CHILD_METADATA
(gdb) p *c
 $2 = {bs = 0x559186f60190, name = 0x559186f669d0 "write-target", klass
     = 0x5591852f3b00 <child_vvfat_qcow>, role = 3, opaque =
         0x559186f3d690, perm = 3, shared_perm = 4, frozen = false,
         parent_quiesce_counter = 0, next = {le_next = 0x0, le_prev =
             0x559186f41818}, next_parent = {le_next = 0x0, le_prev =
                 0x559186f64320}}
(gdb) p s->qcow
 $3 = (BdrvChild *) 0x0

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/vvfat.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 07232a7cfc..86d99c899c 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3209,15 +3209,12 @@ static void vvfat_child_perm(BlockDriverState *bs, BdrvChild *c,
                              uint64_t perm, uint64_t shared,
                              uint64_t *nperm, uint64_t *nshared)
 {
-    BDRVVVFATState *s = bs->opaque;
-
-    assert(c == s->qcow || (role & BDRV_CHILD_COW));
-
-    if (c == s->qcow) {
+    if (role & BDRV_CHILD_DATA) {
         /* This is a private node, nobody should try to attach to it */
         *nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
         *nshared = BLK_PERM_WRITE_UNCHANGED;
     } else {
+        assert(role & BDRV_CHILD_COW);
         /* The backing file is there so 'commit' can use it. vvfat doesn't
          * access it in any way. */
         *nperm = 0;
-- 
2.29.2

Re: [PATCH 0/2] vvfat: fix two crashes.

[Programmingkid]

> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> Hi!
> 
> As reported by Programmingkid, command 
> 
> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
> 
> crashes.
> 
> I tested it with qemu-system-x86_64 and it reproduces for me. I even
> kept "<path of a host folder>" as is :).
> 
> So, here are two fixes.
> 
> Vladimir Sementsov-Ogievskiy (2):
>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>  block/vvfat: fix vvfat_child_perm crash
> 
> include/block/block.h | 1 +
> block.c               | 4 ++--
> block/vvfat.c         | 8 +++-----
> 3 files changed, 6 insertions(+), 7 deletions(-)
> 
> -- 
> 2.29.2

I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.

Thank you.

Reviewed-by: John Arbuckle <programmingkidx@gmail.com>

Re: [PATCH 0/2] vvfat: fix two crashes.

[Vladimir Sementsov-Ogievskiy]

24.05.2021 18:41, Programmingkid wrote:
> 
> 
>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>> Hi!
>>
>> As reported by Programmingkid, command
>>
>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>
>> crashes.
>>
>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>> kept "<path of a host folder>" as is :).
>>
>> So, here are two fixes.
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>   block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>   block/vvfat: fix vvfat_child_perm crash
>>
>> include/block/block.h | 1 +
>> block.c               | 4 ++--
>> block/vvfat.c         | 8 +++-----
>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> -- 
>> 2.29.2
> 
> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
> 
> Thank you.
> 
> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
> 

Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.

Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.

-- 
Best regards,
Vladimir

Re: [PATCH 0/2] vvfat: fix two crashes.

[Programmingkid]

> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> 24.05.2021 18:41, Programmingkid wrote:
>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> 
>>> Hi!
>>> 
>>> As reported by Programmingkid, command
>>> 
>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>> 
>>> crashes.
>>> 
>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>> kept "<path of a host folder>" as is :).
>>> 
>>> So, here are two fixes.
>>> 
>>> Vladimir Sementsov-Ogievskiy (2):
>>>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>  block/vvfat: fix vvfat_child_perm crash
>>> 
>>> include/block/block.h | 1 +
>>> block.c               | 4 ++--
>>> block/vvfat.c         | 8 +++-----
>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>> 
>>> -- 
>>> 2.29.2
>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>> Thank you.
>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
> 
> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.

Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>

> 
> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.

That should be fine but it isn't important.

On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.

Re: [PATCH 0/2] vvfat: fix two crashes.

[Vladimir Sementsov-Ogievskiy]

24.05.2021 19:06, Programmingkid wrote:
> 
> 
>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>> 24.05.2021 18:41, Programmingkid wrote:
>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> Hi!
>>>>
>>>> As reported by Programmingkid, command
>>>>
>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>
>>>> crashes.
>>>>
>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>> kept "<path of a host folder>" as is :).
>>>>
>>>> So, here are two fixes.
>>>>
>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>   block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>   block/vvfat: fix vvfat_child_perm crash
>>>>
>>>> include/block/block.h | 1 +
>>>> block.c               | 4 ++--
>>>> block/vvfat.c         | 8 +++-----
>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>
>>>> -- 
>>>> 2.29.2
>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>> Thank you.
>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>
>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
> 
> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
> 
>>
>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
> 
> That should be fine but it isn't important.
> 
> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
> 

No, I don't know..

Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..

Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?

-- 
Best regards,
Vladimir

Re: [PATCH 0/2] vvfat: fix two crashes.

[Programmingkid]

> On May 24, 2021, at 12:56 PM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> 24.05.2021 19:06, Programmingkid wrote:
>>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> 
>>> 24.05.2021 18:41, Programmingkid wrote:
>>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>> 
>>>>> Hi!
>>>>> 
>>>>> As reported by Programmingkid, command
>>>>> 
>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>> 
>>>>> crashes.
>>>>> 
>>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>>> kept "<path of a host folder>" as is :).
>>>>> 
>>>>> So, here are two fixes.
>>>>> 
>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>>  block/vvfat: fix vvfat_child_perm crash
>>>>> 
>>>>> include/block/block.h | 1 +
>>>>> block.c               | 4 ++--
>>>>> block/vvfat.c         | 8 +++-----
>>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>> 
>>>>> -- 
>>>>> 2.29.2
>>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>>> Thank you.
>>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>> 
>>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
>> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
>>> 
>>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
>> That should be fine but it isn't important.
>> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
> 
> No, I don't know..
> 
> Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..
> 
> Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?

I do actually need it for some of my VM's. It makes sharing of folder between the host and guest so easy. 

Re: [PATCH 0/2] vvfat: fix two crashes.

[Vladimir Sementsov-Ogievskiy]

24.05.2021 20:33, Programmingkid wrote:
> 
> 
>> On May 24, 2021, at 12:56 PM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>> 24.05.2021 19:06, Programmingkid wrote:
>>>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> 24.05.2021 18:41, Programmingkid wrote:
>>>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> As reported by Programmingkid, command
>>>>>>
>>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>>>
>>>>>> crashes.
>>>>>>
>>>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>>>> kept "<path of a host folder>" as is :).
>>>>>>
>>>>>> So, here are two fixes.
>>>>>>
>>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>>   block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>>>   block/vvfat: fix vvfat_child_perm crash
>>>>>>
>>>>>> include/block/block.h | 1 +
>>>>>> block.c               | 4 ++--
>>>>>> block/vvfat.c         | 8 +++-----
>>>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.29.2
>>>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>>>> Thank you.
>>>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>>>
>>>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
>>> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
>>>>
>>>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
>>> That should be fine but it isn't important.
>>> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
>>
>> No, I don't know..
>>
>> Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..
>>
>> Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?
> 
> I do actually need it for some of my VM's. It makes sharing of folder between the host and guest so easy.
> 

Probably virtio-fs is modern way for such thing:

https://virtio-fs.gitlab.io/

-- 
Best regards,
Vladimir

Re: [PATCH 0/2] vvfat: fix two crashes.

[Programmingkid]

> On May 25, 2021, at 2:05 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> 24.05.2021 20:33, Programmingkid wrote:
>>> On May 24, 2021, at 12:56 PM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> 
>>> 24.05.2021 19:06, Programmingkid wrote:
>>>>> On May 24, 2021, at 11:55 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>> 
>>>>> 24.05.2021 18:41, Programmingkid wrote:
>>>>>>> On May 24, 2021, at 6:12 AM, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>>> 
>>>>>>> Hi!
>>>>>>> 
>>>>>>> As reported by Programmingkid, command
>>>>>>> 
>>>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>>>> 
>>>>>>> crashes.
>>>>>>> 
>>>>>>> I tested it with qemu-system-x86_64 and it reproduces for me. I even
>>>>>>> kept "<path of a host folder>" as is :).
>>>>>>> 
>>>>>>> So, here are two fixes.
>>>>>>> 
>>>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>>>  block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash
>>>>>>>  block/vvfat: fix vvfat_child_perm crash
>>>>>>> 
>>>>>>> include/block/block.h | 1 +
>>>>>>> block.c               | 4 ++--
>>>>>>> block/vvfat.c         | 8 +++-----
>>>>>>> 3 files changed, 6 insertions(+), 7 deletions(-)
>>>>>>> 
>>>>>>> -- 
>>>>>>> 2.29.2
>>>>>> I applied both of your patches to the most recent git commit today and they worked. I was able to share files from the host on the guest.
>>>>>> Thank you.
>>>>>> Reviewed-by: John Arbuckle <programmingkidx@gmail.com>
>>>>> 
>>>>> Thanks for testing! Didn't you mean "Tested-by: "? Or if you both reviewed and tested, having both marks makes sense.
>>>> Yes, you are right. It should be: Tested-by: John Arbuckle <programmingkidx@gmail.com>
>>>>> 
>>>>> Hmm, also, I think "Reported-by" in first patch should be fixed to your real name too for consistency.
>>>> That should be fine but it isn't important.
>>>> On a related topic would you know if it is possible to use fat32 instead of fat16 for host folder sharing? I did try replacing the text fat16 with fat32 but it didn't appear to work.
>>> 
>>> No, I don't know..
>>> 
>>> Moreover, my quick look at the code of vvfat, this fixed bug (which is obviously not covered by tests), and also status of block/vvfat in MAINTAINERS file "Odd Fixes", all this leads to advice "don't use it if possible". May be Kevin can add something about it, he is maintainer..
>>> 
>>> Could you use for example NFS or Samba, or sshfs to share folders? Or you need exactly to make a host folder available in guest vm as usb drive?
>> I do actually need it for some of my VM's. It makes sharing of folder between the host and guest so easy.
> 
> Probably virtio-fs is modern way for such thing:
> 
> https://virtio-fs.gitlab.io/

Thank you for this.

Re: [PATCH 0/2] vvfat: fix two crashes.

[Kevin Wolf]

Am 24.05.2021 um 18:06 hat Programmingkid geschrieben:
> >>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
> >>> 
> On a related topic would you know if it is possible to use fat32
> instead of fat16 for host folder sharing? I did try replacing the text
> fat16 with fat32 but it didn't appear to work.

I think the correct syntax is fat:32:rw:<path>. But one of the first
things it does is:

    warn_report("FAT32 has not been tested. You are welcome to do so!");

So probably nobody would be surprised if it broke.

Kevin

Re: [PATCH 0/2] vvfat: fix two crashes.

[Programmingkid]

> On May 25, 2021, at 12:18 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> Am 24.05.2021 um 18:06 hat Programmingkid geschrieben:
>>>>> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
>>>>> 
>> On a related topic would you know if it is possible to use fat32
>> instead of fat16 for host folder sharing? I did try replacing the text
>> fat16 with fat32 but it didn't appear to work.
> 
> I think the correct syntax is fat:32:rw:<path>. But one of the first
> things it does is:
> 
>    warn_report("FAT32 has not been tested. You are welcome to do so!");
> 
> So probably nobody would be surprised if it broke.
> 
> Kevin
> 

Thank you very much for this information. 

Re: [PATCH 0/2] vvfat: fix two crashes.

[Kevin Wolf]

Am 24.05.2021 um 12:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi!
> 
> As reported by Programmingkid, command 
> 
> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none
> 
> crashes.
> 
> I tested it with qemu-system-x86_64 and it reproduces for me. I even
> kept "<path of a host folder>" as is :).
> 
> So, here are two fixes.

Thanks, applied to the block branch.

Kevin