* [PATCH 0/2] Fix some problems with vvfat in R/W mode
@ 2022-09-03 16:23 Hervé Poussineau
2022-09-03 16:23 ` [PATCH 1/2] vvfat: allow some writes to bootsector Hervé Poussineau
2022-09-03 16:23 ` [PATCH 2/2] vvfat: allow spaces in file names Hervé Poussineau
0 siblings, 2 replies; 8+ messages in thread
From: Hervé Poussineau @ 2022-09-03 16:23 UTC (permalink / raw)
To: Kevin Wolf, Hanna Reitz, qemu-block; +Cc: qemu-devel, Hervé Poussineau
Hi,
When testing vvfat in read-write mode, I came across some blocking
problems when using Windows guests.
This patchset is not here to fix all problems of vvfat, but only the
main ones I encountered.
First patch allows setting/resetting the 'volume dirty' flag on
boosector, and the second one allows creating file names with spaces.
Hervé
Hervé Poussineau (2):
vvfat: allow some writes to bootsector
vvfat: allow spaces in file names
block/vvfat.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
--
2.36.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] vvfat: allow some writes to bootsector
2022-09-03 16:23 [PATCH 0/2] Fix some problems with vvfat in R/W mode Hervé Poussineau
@ 2022-09-03 16:23 ` Hervé Poussineau
2022-09-29 14:10 ` Kevin Wolf
2022-09-03 16:23 ` [PATCH 2/2] vvfat: allow spaces in file names Hervé Poussineau
1 sibling, 1 reply; 8+ messages in thread
From: Hervé Poussineau @ 2022-09-03 16:23 UTC (permalink / raw)
To: Kevin Wolf, Hanna Reitz, qemu-block; +Cc: qemu-devel, Hervé Poussineau
'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
Allow writes to bootsector which only changes the 'reserved1' field.
This fixes I/O errors on Windows guests.
Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
block/vvfat.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683d..35057a51c67 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2993,11 +2993,27 @@ DLOG(checkpoint());
vvfat_close_current_file(s);
+ if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
+ /*
+ * Write on bootsector. Allow only changing the reserved1 field,
+ * used to mark volume dirtiness
+ */
+ const unsigned char *initial = s->first_sectors
+ + s->offset_to_bootsector * 0x200;
+ for (i = 0; i < 0x200; i++) {
+ if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
+ initial[i] != buf[i]) {
+ fprintf(stderr, "Tried to write to protected bootsector\n");
+ return -1;
+ }
+ }
+ return 0;
+ }
+
/*
* Some sanity checks:
* - do not allow writing to the boot sector
*/
-
if (sector_num < s->offset_to_fat)
return -1;
--
2.36.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] vvfat: allow some writes to bootsector
2022-09-03 16:23 ` [PATCH 1/2] vvfat: allow some writes to bootsector Hervé Poussineau
@ 2022-09-29 14:10 ` Kevin Wolf
2022-09-29 19:53 ` Hervé Poussineau
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2022-09-29 14:10 UTC (permalink / raw)
To: Hervé Poussineau; +Cc: Hanna Reitz, qemu-block, qemu-devel
Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
> 'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
> Allow writes to bootsector which only changes the 'reserved1' field.
>
> This fixes I/O errors on Windows guests.
>
> Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
> block/vvfat.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683d..35057a51c67 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2993,11 +2993,27 @@ DLOG(checkpoint());
>
> vvfat_close_current_file(s);
>
> + if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
> + /*
> + * Write on bootsector. Allow only changing the reserved1 field,
> + * used to mark volume dirtiness
> + */
> + const unsigned char *initial = s->first_sectors
> + + s->offset_to_bootsector * 0x200;
> + for (i = 0; i < 0x200; i++) {
> + if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
I think you need to check the FAT version (s->fat_type) before accessing
u.fat16. For FAT32, the "reserved" field is at a different offset (but
seems to have the same meaning).
> + initial[i] != buf[i]) {
> + fprintf(stderr, "Tried to write to protected bootsector\n");
> + return -1;
> + }
> + }
> + return 0;
> + }
Should we update s->first_sectors with the new value so that the guest
would actually read back what it wrote instead of having the change
disappear magically?
> /*
> * Some sanity checks:
> * - do not allow writing to the boot sector
> */
> -
> if (sector_num < s->offset_to_fat)
> return -1;
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] vvfat: allow some writes to bootsector
2022-09-29 14:10 ` Kevin Wolf
@ 2022-09-29 19:53 ` Hervé Poussineau
2022-09-30 9:57 ` Kevin Wolf
0 siblings, 1 reply; 8+ messages in thread
From: Hervé Poussineau @ 2022-09-29 19:53 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Hanna Reitz, qemu-block, qemu-devel
Le 29/09/2022 à 16:10, Kevin Wolf a écrit :
> Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
>> 'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
>> Allow writes to bootsector which only changes the 'reserved1' field.
>>
>> This fixes I/O errors on Windows guests.
>>
>> Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>> block/vvfat.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index d6dd919683d..35057a51c67 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2993,11 +2993,27 @@ DLOG(checkpoint());
>>
>> vvfat_close_current_file(s);
>>
>> + if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
>> + /*
>> + * Write on bootsector. Allow only changing the reserved1 field,
>> + * used to mark volume dirtiness
>> + */
>> + const unsigned char *initial = s->first_sectors
>> + + s->offset_to_bootsector * 0x200;
>> + for (i = 0; i < 0x200; i++) {
>> + if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
>
> I think you need to check the FAT version (s->fat_type) before accessing
> u.fat16. For FAT32, the "reserved" field is at a different offset (but
> seems to have the same meaning).
I didn't do this, because only fat16 part of bootsector is ever used.
In init_directories(), only fat16 part is initialized, with the comment:
/* LATER TODO: if FAT32, this is wrong */
I wanted to be consistent between init_directories() and the check.
>
>> + initial[i] != buf[i]) {
>> + fprintf(stderr, "Tried to write to protected bootsector\n");
>> + return -1;
>> + }
>> + }
>> + return 0;
>> + }
>
> Should we update s->first_sectors with the new value so that the guest
> would actually read back what it wrote instead of having the change
> disappear magically?
Windows guests don't seem to care if the written value disappears. They only want the write to succeed.
>
>> /*
>> * Some sanity checks:
>> * - do not allow writing to the boot sector
>> */
>> -
>> if (sector_num < s->offset_to_fat)
>> return -1;
>
> Kevin
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] vvfat: allow some writes to bootsector
2022-09-29 19:53 ` Hervé Poussineau
@ 2022-09-30 9:57 ` Kevin Wolf
0 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2022-09-30 9:57 UTC (permalink / raw)
To: Hervé Poussineau; +Cc: Hanna Reitz, qemu-block, qemu-devel
Am 29.09.2022 um 21:53 hat Hervé Poussineau geschrieben:
> Le 29/09/2022 à 16:10, Kevin Wolf a écrit :
> > Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
> > > 'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
> > > Allow writes to bootsector which only changes the 'reserved1' field.
> > >
> > > This fixes I/O errors on Windows guests.
> > >
> > > Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
> > > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> > > ---
> > > block/vvfat.c | 18 +++++++++++++++++-
> > > 1 file changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > index d6dd919683d..35057a51c67 100644
> > > --- a/block/vvfat.c
> > > +++ b/block/vvfat.c
> > > @@ -2993,11 +2993,27 @@ DLOG(checkpoint());
> > > vvfat_close_current_file(s);
> > > + if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
> > > + /*
> > > + * Write on bootsector. Allow only changing the reserved1 field,
> > > + * used to mark volume dirtiness
> > > + */
> > > + const unsigned char *initial = s->first_sectors
> > > + + s->offset_to_bootsector * 0x200;
> > > + for (i = 0; i < 0x200; i++) {
> > > + if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
> >
> > I think you need to check the FAT version (s->fat_type) before accessing
> > u.fat16. For FAT32, the "reserved" field is at a different offset (but
> > seems to have the same meaning).
>
> I didn't do this, because only fat16 part of bootsector is ever used.
> In init_directories(), only fat16 part is initialized, with the comment:
> /* LATER TODO: if FAT32, this is wrong */
> I wanted to be consistent between init_directories() and the check.
Oh, indeed. I guess this means FAT32 is completely broken... Fair
enough, though maybe we could add a similar comment here, then.
> > > + initial[i] != buf[i]) {
> > > + fprintf(stderr, "Tried to write to protected bootsector\n");
> > > + return -1;
> > > + }
> > > + }
> > > + return 0;
> > > + }
> >
> > Should we update s->first_sectors with the new value so that the guest
> > would actually read back what it wrote instead of having the change
> > disappear magically?
>
> Windows guests don't seem to care if the written value disappears.
> They only want the write to succeed.
But it would be arguably more correct, wouldn't it? Some other OS might
care.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] vvfat: allow spaces in file names
2022-09-03 16:23 [PATCH 0/2] Fix some problems with vvfat in R/W mode Hervé Poussineau
2022-09-03 16:23 ` [PATCH 1/2] vvfat: allow some writes to bootsector Hervé Poussineau
@ 2022-09-03 16:23 ` Hervé Poussineau
2022-09-04 13:45 ` Philippe Mathieu-Daudé via
2022-09-29 14:24 ` Kevin Wolf
1 sibling, 2 replies; 8+ messages in thread
From: Hervé Poussineau @ 2022-09-03 16:23 UTC (permalink / raw)
To: Kevin Wolf, Hanna Reitz, qemu-block; +Cc: qemu-devel, Hervé Poussineau
In R/W mode, files with spaces were never created on host side.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1176
Fixes: c79e243ed67683d6d06692bd7040f7394da178b0
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
block/vvfat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 35057a51c67..9d877028573 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -499,7 +499,7 @@ static bool valid_filename(const unsigned char *name)
(c >= 'A' && c <= 'Z') ||
(c >= 'a' && c <= 'z') ||
c > 127 ||
- strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
+ strchr(" $%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
{
return false;
}
--
2.36.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] vvfat: allow spaces in file names
2022-09-03 16:23 ` [PATCH 2/2] vvfat: allow spaces in file names Hervé Poussineau
@ 2022-09-04 13:45 ` Philippe Mathieu-Daudé via
2022-09-29 14:24 ` Kevin Wolf
1 sibling, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-04 13:45 UTC (permalink / raw)
To: Hervé Poussineau, Kevin Wolf, Hanna Reitz, qemu-block; +Cc: qemu-devel
On 3/9/22 18:23, Hervé Poussineau wrote:
> In R/W mode, files with spaces were never created on host side.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1176
> Fixes: c79e243ed67683d6d06692bd7040f7394da178b0
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
> block/vvfat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 35057a51c67..9d877028573 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -499,7 +499,7 @@ static bool valid_filename(const unsigned char *name)
> (c >= 'A' && c <= 'Z') ||
> (c >= 'a' && c <= 'z') ||
> c > 127 ||
> - strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
> + strchr(" $%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
> {
> return false;
> }
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] vvfat: allow spaces in file names
2022-09-03 16:23 ` [PATCH 2/2] vvfat: allow spaces in file names Hervé Poussineau
2022-09-04 13:45 ` Philippe Mathieu-Daudé via
@ 2022-09-29 14:24 ` Kevin Wolf
1 sibling, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2022-09-29 14:24 UTC (permalink / raw)
To: Hervé Poussineau; +Cc: Hanna Reitz, qemu-block, qemu-devel
Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
> In R/W mode, files with spaces were never created on host side.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1176
> Fixes: c79e243ed67683d6d06692bd7040f7394da178b0
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-30 9:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-03 16:23 [PATCH 0/2] Fix some problems with vvfat in R/W mode Hervé Poussineau
2022-09-03 16:23 ` [PATCH 1/2] vvfat: allow some writes to bootsector Hervé Poussineau
2022-09-29 14:10 ` Kevin Wolf
2022-09-29 19:53 ` Hervé Poussineau
2022-09-30 9:57 ` Kevin Wolf
2022-09-03 16:23 ` [PATCH 2/2] vvfat: allow spaces in file names Hervé Poussineau
2022-09-04 13:45 ` Philippe Mathieu-Daudé via
2022-09-29 14:24 ` 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.