* [PATCH] block/vvfat: Fix ro shared folder
@ 2021-08-31 14:17 Guillaume Roche
2021-08-31 16:06 ` Philippe Mathieu-Daudé
2021-09-08 14:11 ` Kevin Wolf
0 siblings, 2 replies; 4+ messages in thread
From: Guillaume Roche @ 2021-08-31 14:17 UTC (permalink / raw)
Cc: Guillaume Roche, Kevin Wolf, Hanna Reitz,
open list:Block layer core, open list:All patches CC here
QEMU exits in error when passing a vfat shared folder in read-only mode.
To fix this issue, this patch removes any potential write permission
from cumulative_perms, when a read-only block device is in use.
Buglink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918950
Signed-off-by: Guillaume Roche <groche@genymobile.com>
---
This is an attempt to fix this behavior, but it feels a bit hacky to me
since this patch checks for the vvfat format in a generic function.
However, I'd be glad to have some advice to make it better. Anyway, I
ran the block tests to ensure this does not introduce any regression.
To add some context: I know that this can be worked around by setting
the shared folder in rw mode. But our use-case requires using both
shared and VM snapshots, and QEMU prevents using snapshot with a rw
shared folder.
block.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/block.c b/block.c
index e97ce0b1c8..3f59e3843f 100644
--- a/block.c
+++ b/block.c
@@ -2383,6 +2383,12 @@ void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
cumulative_shared_perms &= c->shared_perm;
}
+ /* Discard write permission if vvfat block device is read-only */
+ const char *format = bdrv_get_format_name(bs);
+ if (format != NULL && strncmp(format, "vvfat", 5) == 0 && bdrv_is_read_only(bs)) {
+ cumulative_perms &= ~BLK_PERM_WRITE;
+ }
+
*perm = cumulative_perms;
*shared_perm = cumulative_shared_perms;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] block/vvfat: Fix ro shared folder
2021-08-31 14:17 [PATCH] block/vvfat: Fix ro shared folder Guillaume Roche
@ 2021-08-31 16:06 ` Philippe Mathieu-Daudé
2021-09-02 15:41 ` Guillaume Roche
2021-09-08 14:11 ` Kevin Wolf
1 sibling, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-31 16:06 UTC (permalink / raw)
To: Guillaume Roche
Cc: Kevin Wolf, Hanna Reitz, open list:All patches CC here,
open list:Block layer core
Hi Guillaume,
On 8/31/21 4:17 PM, Guillaume Roche wrote:
> QEMU exits in error when passing a vfat shared folder in read-only mode.
>
> To fix this issue, this patch removes any potential write permission
> from cumulative_perms, when a read-only block device is in use.
>
> Buglink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918950
>
> Signed-off-by: Guillaume Roche <groche@genymobile.com>
> ---
> This is an attempt to fix this behavior, but it feels a bit hacky to me
> since this patch checks for the vvfat format in a generic function.
What about implementing bdrv_vvfat::bdrv_check_perm()?
> However, I'd be glad to have some advice to make it better. Anyway, I
> ran the block tests to ensure this does not introduce any regression.
>
> To add some context: I know that this can be worked around by setting
> the shared folder in rw mode. But our use-case requires using both
> shared and VM snapshots, and QEMU prevents using snapshot with a rw
> shared folder.
>
> block.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/block.c b/block.c
> index e97ce0b1c8..3f59e3843f 100644
> --- a/block.c
> +++ b/block.c
> @@ -2383,6 +2383,12 @@ void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
> cumulative_shared_perms &= c->shared_perm;
> }
>
> + /* Discard write permission if vvfat block device is read-only */
> + const char *format = bdrv_get_format_name(bs);
> + if (format != NULL && strncmp(format, "vvfat", 5) == 0 && bdrv_is_read_only(bs)) {
> + cumulative_perms &= ~BLK_PERM_WRITE;
> + }
> +
> *perm = cumulative_perms;
> *shared_perm = cumulative_shared_perms;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] block/vvfat: Fix ro shared folder
2021-08-31 16:06 ` Philippe Mathieu-Daudé
@ 2021-09-02 15:41 ` Guillaume Roche
0 siblings, 0 replies; 4+ messages in thread
From: Guillaume Roche @ 2021-09-02 15:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Hanna Reitz, open list:Block layer core,
open list:All patches CC here
Hi Philippe,
Le mar. 31 août 2021 à 18:06, Philippe Mathieu-Daudé
<philmd@redhat.com> a écrit :
>
> Hi Guillaume,
>
> On 8/31/21 4:17 PM, Guillaume Roche wrote:
> > QEMU exits in error when passing a vfat shared folder in read-only mode.
> >
> > To fix this issue, this patch removes any potential write permission
> > from cumulative_perms, when a read-only block device is in use.
> >
> > Buglink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918950
> >
> > Signed-off-by: Guillaume Roche <groche@genymobile.com>
> > ---
> > This is an attempt to fix this behavior, but it feels a bit hacky to me
> > since this patch checks for the vvfat format in a generic function.
>
> What about implementing bdrv_vvfat::bdrv_check_perm()?
Thanks for this feedback. I had a look at your suggestion, but I'm a
bit confused.
As I understand it, bdrv_node_refresh_perm() calls
bdrv_get_cumulative_perm() (which I patched to remove the write
permission) then it checks the permissions.
Afterwards, it calls bdrv_drv_set_perm() that in turn calls
bs->drv->bdrv_check_perm(). So even if I implement
bdrv_vvfat::bdrv_check_perm(), I will get the permission error before
it is called.
Could you elaborate a bit on what you have in mind please?
Regards,
Guillaume
>
> > However, I'd be glad to have some advice to make it better. Anyway, I
> > ran the block tests to ensure this does not introduce any regression.
> >
> > To add some context: I know that this can be worked around by setting
> > the shared folder in rw mode. But our use-case requires using both
> > shared and VM snapshots, and QEMU prevents using snapshot with a rw
> > shared folder.
> >
> > block.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index e97ce0b1c8..3f59e3843f 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2383,6 +2383,12 @@ void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
> > cumulative_shared_perms &= c->shared_perm;
> > }
> >
> > + /* Discard write permission if vvfat block device is read-only */
> > + const char *format = bdrv_get_format_name(bs);
> > + if (format != NULL && strncmp(format, "vvfat", 5) == 0 && bdrv_is_read_only(bs)) {
> > + cumulative_perms &= ~BLK_PERM_WRITE;
> > + }
> > +
> > *perm = cumulative_perms;
> > *shared_perm = cumulative_shared_perms;
> > }
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] block/vvfat: Fix ro shared folder
2021-08-31 14:17 [PATCH] block/vvfat: Fix ro shared folder Guillaume Roche
2021-08-31 16:06 ` Philippe Mathieu-Daudé
@ 2021-09-08 14:11 ` Kevin Wolf
1 sibling, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2021-09-08 14:11 UTC (permalink / raw)
To: Guillaume Roche
Cc: Hanna Reitz, open list:All patches CC here, open list:Block layer core
Am 31.08.2021 um 16:17 hat Guillaume Roche geschrieben:
> QEMU exits in error when passing a vfat shared folder in read-only mode.
>
> To fix this issue, this patch removes any potential write permission
> from cumulative_perms, when a read-only block device is in use.
>
> Buglink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918950
>
> Signed-off-by: Guillaume Roche <groche@genymobile.com>
> ---
> This is an attempt to fix this behavior, but it feels a bit hacky to me
> since this patch checks for the vvfat format in a generic function.
>
> However, I'd be glad to have some advice to make it better. Anyway, I
> ran the block tests to ensure this does not introduce any regression.
>
> To add some context: I know that this can be worked around by setting
> the shared folder in rw mode. But our use-case requires using both
> shared and VM snapshots, and QEMU prevents using snapshot with a rw
> shared folder.
I don't think the behaviour is actually a bug: ide-hd requires a
writable backend, so attaching a read-only vvfat node is a real error.
You can either specify -drive read-only=on and use a device that can
accept read-only backends (such as virtio-blk or ide-cd), or add a
temporary writable qcow2 overlay with -snapshot or -drive snapshot=on so
that the ide-hd device actually does get the writable backend it needs.
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-08 14:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 14:17 [PATCH] block/vvfat: Fix ro shared folder Guillaume Roche
2021-08-31 16:06 ` Philippe Mathieu-Daudé
2021-09-02 15:41 ` Guillaume Roche
2021-09-08 14:11 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.