[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] vvfat: allow some writes to bootsector
From: |
Kevin Wolf |
Subject: |
Re: [PATCH 1/2] vvfat: allow some writes to bootsector |
Date: |
Fri, 30 Sep 2022 11:57:16 +0200 |
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