Re: [Qemu-devel] [PATCH 05/13] vvfat: introduce offset_to_bootsector, offset_to_fat and offset_to_root_dir

[Hervé Poussineau]

Le 16/05/2017 à 16:16, Kevin Wolf a écrit :
> Am 15.05.2017 um 22:31 hat Hervé Poussineau geschrieben:
> > - offset_to_bootsector is the number of sectors up to FAT bootsector
> > - offset_to_fat is the number of sectors up to first File Allocation Table
> > - offset_to_root_dir is the number of sectors up to root directory sector
>
> Hm... These names make me think of byte offsets. Not completely opposed
> to them, but if anyone can think of something better...?
>
> > Replace first_sectors_number - 1 by offset_to_bootsector.
> > Replace first_sectors_number by offset_to_fat.
> > Replace faked_sectors by offset_to_rootdir.
> >
> > Signed-off-by: Hervé Poussineau <address@hidden>
> > ---
> >  block/vvfat.c | 67 +++++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 40 insertions(+), 27 deletions(-)
> >
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 4f4a63c03f..f60d2a3889 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -320,22 +320,24 @@ static void print_mapping(const struct mapping_t* 
> > mapping);
> >  typedef struct BDRVVVFATState {
> >      CoMutex lock;
> >      BlockDriverState* bs; /* pointer to parent */
> > -    unsigned int first_sectors_number; /* 1 for a single partition, 0x40 for a 
> > disk with partition table */
> >      unsigned char first_sectors[0x40*0x200];
> >
> >      int fat_type; /* 16 or 32 */
> >      array_t fat,directory,mapping;
> >      char volume_label[11];
> >
> > +    uint32_t offset_to_bootsector; /* 0 for floppy, 0x3f for disk */
> > +
> >      unsigned int cluster_size;
> >      unsigned int sectors_per_cluster;
> >      unsigned int sectors_per_fat;
> >      unsigned int sectors_of_root_directory;
> >      uint32_t last_cluster_of_root_directory;
> > -    unsigned int faked_sectors; /* how many sectors are faked before file data 
> > */
> >      uint32_t sector_count; /* total number of sectors of the partition */
> >      uint32_t cluster_count; /* total number of clusters of this partition */
> >      uint32_t max_fat_value;
> > +    uint32_t offset_to_fat;
> > +    uint32_t offset_to_root_dir;
> >
> >      int current_fd;
> >      mapping_t* current_mapping;
> > @@ -394,15 +396,15 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int 
> > heads, int secs)
> >      partition->attributes=0x80; /* bootable */
> >
> >      /* LBA is used when partition is outside the CHS geometry */
> > -    lba  = sector2CHS(&partition->start_CHS, s->first_sectors_number - 1,
> > +    lba  = sector2CHS(&partition->start_CHS, s->offset_to_bootsector,
> >                       cyls, heads, secs);
> >      lba |= sector2CHS(&partition->end_CHS,   s->bs->total_sectors - 1,
> >                       cyls, heads, secs);
> >
> >      /*LBA partitions are identified only by start/length_sector_long not by 
> > CHS*/
> > -    partition->start_sector_long  = cpu_to_le32(s->first_sectors_number - 1);
> > +    partition->start_sector_long  = cpu_to_le32(s->offset_to_bootsector);
> >      partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
> > -                                                - s->first_sectors_number + 1);
> > +                                                - s->offset_to_bootsector);
> >
> >      /* FAT12/FAT16/FAT32 */
> >      /* DOS uses different types when partition is LBA,
> > @@ -823,12 +825,12 @@ static int read_directory(BDRVVVFATState* s, int 
> > mapping_index)
> >
> >  static inline uint32_t sector2cluster(BDRVVVFATState* s,off_t sector_num)
> >  {
> > -    return (sector_num-s->faked_sectors)/s->sectors_per_cluster;
> > +    return (sector_num - s->offset_to_root_dir) / s->sectors_per_cluster;
> >  }
> >
> >  static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num)
> >  {
> > -    return s->faked_sectors + s->sectors_per_cluster * cluster_num;
> > +    return s->offset_to_root_dir + s->sectors_per_cluster * cluster_num;
> >  }
> >
> >  static int init_directories(BDRVVVFATState* s,
> > @@ -855,6 +857,9 @@ static int init_directories(BDRVVVFATState* s,
> >      i = 1+s->sectors_per_cluster*0x200*8/s->fat_type;
> >      s->sectors_per_fat=(s->sector_count+i)/i; /* round up */
> >
> > +    s->offset_to_fat = s->offset_to_bootsector + 1;
> > +    s->offset_to_root_dir = s->offset_to_fat + s->sectors_per_fat * 2;
> > +
> >      array_init(&(s->mapping),sizeof(mapping_t));
> >      array_init(&(s->directory),sizeof(direntry_t));
> >
> > @@ -868,7 +873,6 @@ static int init_directories(BDRVVVFATState* s,
> >      /* Now build FAT, and write back information into directory */
> >      init_fat(s);
> >
> > -    s->faked_sectors=s->first_sectors_number+s->sectors_per_fat*2;
> >      s->cluster_count=sector2cluster(s, s->sector_count);
> >
> >      mapping = array_get_next(&(s->mapping));
> > @@ -946,7 +950,8 @@ static int init_directories(BDRVVVFATState* s,
> >
> >      s->current_mapping = NULL;
> >
> > -    
> > bootsector=(bootsector_t*)(s->first_sectors+(s->first_sectors_number-1)*0x200);
> > +    bootsector = (bootsector_t *)(s->first_sectors
> > +                                  + s->offset_to_bootsector * 0x200);
> >      bootsector->jump[0]=0xeb;
> >      bootsector->jump[1]=0x3e;
> >      bootsector->jump[2]=0x90;
> > @@ -957,16 +962,16 @@ static int init_directories(BDRVVVFATState* s,
> >      bootsector->number_of_fats=0x2; /* number of FATs */
> >      bootsector->root_entries=cpu_to_le16(s->sectors_of_root_directory*0x10);
> >      
> > bootsector->total_sectors16=s->sector_count>0xffff?0:cpu_to_le16(s->sector_count);
> > -    bootsector->media_type=(s->first_sectors_number>1?0xf8:0xf0); /* media 
> > descriptor (f8=hd, f0=3.5 fd)*/
> > +    bootsector->media_type = (s->offset_to_bootsector > 0 ? 0xf8 : 0xf0);
>
> Please keep the comment. I don't mind if you want to move it to its own
> line, but it's better to have magic numbers like 0xf8 and 0xf0 explained
> somewhere. (Or you could introduce descriptive #defines for them.)

OK (I keep the comment on a new line)

> >      s->fat.pointer[0] = bootsector->media_type;
> >      bootsector->sectors_per_fat=cpu_to_le16(s->sectors_per_fat);
> >      bootsector->sectors_per_track = cpu_to_le16(secs);
> >      bootsector->number_of_heads = cpu_to_le16(heads);
> > -    bootsector->hidden_sectors=cpu_to_le32(s->first_sectors_number==1?0:0x3f);
> > +    bootsector->hidden_sectors = cpu_to_le32(s->offset_to_bootsector);
> >      
> > bootsector->total_sectors=cpu_to_le32(s->sector_count>0xffff?s->sector_count:0);
>
> Nice simplification. :-)
>
> >      /* LATER TODO: if FAT32, this is wrong */
> > -    bootsector->u.fat16.drive_number=s->first_sectors_number==1?0:0x80; /* 
> > fda=0, hda=0x80 */
> > +    bootsector->u.fat16.drive_number = s->offset_to_bootsector == 0 ? 0 : 0x80;
>
> Here I would like the comment to be preserved again.

OK

> >      bootsector->u.fat16.current_head=0;
> >      bootsector->u.fat16.signature=0x29;
> >      bootsector->u.fat16.id=cpu_to_le32(0xfabe1afd);
> > @@ -1123,7 +1128,6 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >              secs = s->fat_type == 12 ? 18 : 36;
> >              s->sectors_per_cluster = 1;
> >          }
> > -        s->first_sectors_number = 1;
> >          cyls = 80;
> >          heads = 2;
> >      } else {
> > @@ -1131,7 +1135,7 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >          if (!s->fat_type) {
> >              s->fat_type = 16;
> >          }
> > -        s->first_sectors_number = 0x40;
> > +        s->offset_to_bootsector = 0x3f;
> >          cyls = s->fat_type == 12 ? 64 : 1024;
> >          heads = 16;
> >          secs = 63;
> > @@ -1169,7 +1173,7 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >      fprintf(stderr, "vvfat %s chs %d,%d,%d\n",
> >              dirname, cyls, heads, secs);
> >
> > -    s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
> > +    s->sector_count = cyls * heads * secs - s->offset_to_bootsector;
> >
> >      if (qemu_opt_get_bool(opts, "rw", false)) {
> >          ret = enable_write_target(bs, errp);
> > @@ -1186,7 +1190,8 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >          goto fail;
> >      }
> >
> > -    s->sector_count = s->faked_sectors + 
> > s->sectors_per_cluster*s->cluster_count;
> > +    s->sector_count = s->offset_to_root_dir
> > +                    + s->sectors_per_cluster * s->cluster_count;
> >
> >      /* Disable migration when vvfat is used rw */
> >      if (s->qcow) {
> > @@ -1202,7 +1207,7 @@ static int vvfat_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >          }
> >      }
> >
> > -    if (s->first_sectors_number == 0x40) {
> > +    if (s->offset_to_bootsector > 0) {
> >          init_mbr(s, cyls, heads, secs);
> >      }
> >
> > @@ -1415,15 +1420,23 @@ static int vvfat_read(BlockDriverState *bs, int64_t 
> > sector_num,
> >              }
> >  DLOG(fprintf(stderr, "sector %d not allocated\n", (int)sector_num));
> >          }
> > -        if(sector_num<s->faked_sectors) {
> > -            if(sector_num<s->first_sectors_number)
> > -                
> > memcpy(buf+i*0x200,&(s->first_sectors[sector_num*0x200]),0x200);
> > -            else if(sector_num-s->first_sectors_number<s->sectors_per_fat)
> > -                
> > memcpy(buf+i*0x200,&(s->fat.pointer[(sector_num-s->first_sectors_number)*0x200]),0x200);
> > -            else 
> > if(sector_num-s->first_sectors_number-s->sectors_per_fat<s->sectors_per_fat)
> > -                
> > memcpy(buf+i*0x200,&(s->fat.pointer[(sector_num-s->first_sectors_number-s->sectors_per_fat)*0x200]),0x200);
> > +        if (sector_num < s->offset_to_root_dir) {
> > +            if (sector_num < s->offset_to_fat)
> > +                memcpy(buf + i * 0x200,
> > +                       &(s->first_sectors[sector_num * 0x200]),
> > +                       0x200);
> > +            else if (sector_num < s->offset_to_fat + s->sectors_per_fat)
> > +                memcpy(buf + i * 0x200,
> > +                       &(s->fat.pointer[(sector_num
> > +                                       - s->offset_to_fat) * 0x200]),
> > +                       0x200);
> > +            else if (sector_num < s->offset_to_root_dir)
> > +                memcpy(buf + i * 0x200,
> > +                       &(s->fat.pointer[(sector_num - s->offset_to_fat
> > +                                       - s->sectors_per_fat) * 0x200]),
> > +                       0x200);
>
> The QEMU coding style requires braces for all branches of this if
> statement.

checkpatch.pl didn't complain...

Hervé