Patchwork [1/7] block/vpc: Fix most coding style warnings and errors

login
register
mail settings
Submitter Stefan Weil
Date Feb. 1, 2013, 9:51 p.m.
Message ID <1359755494-28012-2-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/217596/
State Under Review
Headers show

Comments

Stefan Weil - Feb. 1, 2013, 9:51 p.m.
Only C99 comments remain unfixed.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 block/vpc.c |   94 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 42 deletions(-)
Kevin Wolf - Feb. 5, 2013, 10:20 a.m.
Am 01.02.2013 22:51, schrieb Stefan Weil:
> Only C99 comments remain unfixed.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  block/vpc.c |   94 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 52 insertions(+), 42 deletions(-)

> @@ -578,8 +589,8 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
>  
>  static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
>  {
> -    struct vhd_dyndisk_header* dyndisk_header =
> -        (struct vhd_dyndisk_header*) buf;
> +    struct vhd_dyndisk_header *dyndisk_header =
> +        (struct vhd_dyndisk_header *)buf;

Please leave the space after the closing bracket.

>      size_t block_size, num_bat_entries;
>      int i;
>      int ret = -EIO;
> @@ -709,8 +720,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
>      total_sectors = total_size / BDRV_SECTOR_SIZE;
>      for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
>          if (calculate_geometry(total_sectors + i, &cyls, &heads,
> -                               &secs_per_cyl))
> -        {
> +                               &secs_per_cyl)) {
>              ret = -EFBIG;
>              goto fail;
>          }

This is not an improvement. CODING_STYLE says:

  The opening brace is on the line that contains the control
  flow statement that introduces the new block

It wasn't on the line of the "if" before the patch, and it isn't after
the patch (and it can't because of the 80 characters/line maximum, which
is probably the more important rule). I like the readabilty of the
existing version better.

Kevin
Stefan Weil - Feb. 5, 2013, 10:45 a.m.
Am 05.02.2013 11:20, schrieb Kevin Wolf:
> Am 01.02.2013 22:51, schrieb Stefan Weil:
>> Only C99 comments remain unfixed.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  block/vpc.c |   94 +++++++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 52 insertions(+), 42 deletions(-)
>> @@ -578,8 +589,8 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
>>  
>>  static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
>>  {
>> -    struct vhd_dyndisk_header* dyndisk_header =
>> -        (struct vhd_dyndisk_header*) buf;
>> +    struct vhd_dyndisk_header *dyndisk_header =
>> +        (struct vhd_dyndisk_header *)buf;
> Please leave the space after the closing bracket.

$ git grep ' \*) [a-z]'|wc
    858    6568   59803
$ git grep ' \*)[a-z]'|wc
   1469    9107  101610

$ git grep ' \*) [a-z]' block|wc
      7      55     499
$ git grep ' \*)[a-z]' block|wc
     52     322    3514

The majority of code does not use a space in pointer type casts.
For block/vpc.c, there were 4 such type casts using a space and
one without.

Here the line was modified because of a CODING_STYLE violation,
and I additionally removed the space to be compatible with
the other code in QEMU and especially in block/*.

If you want the space nevertheless, I'll leave it there.

>>      size_t block_size, num_bat_entries;
>>      int i;
>>      int ret = -EIO;
>> @@ -709,8 +720,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
>>      total_sectors = total_size / BDRV_SECTOR_SIZE;
>>      for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
>>          if (calculate_geometry(total_sectors + i, &cyls, &heads,
>> -                               &secs_per_cyl))
>> -        {
>> +                               &secs_per_cyl)) {
>>              ret = -EFBIG;
>>              goto fail;
>>          }
> This is not an improvement. CODING_STYLE says:
>
>   The opening brace is on the line that contains the control
>   flow statement that introduces the new block
>
> It wasn't on the line of the "if" before the patch, and it isn't after
> the patch (and it can't because of the 80 characters/line maximum, which
> is probably the more important rule). I like the readabilty of the
> existing version better.
>
> Kevin


This is what checkpatch.pl says:

ERROR: that open brace { should be on the previous line
#729: FILE: vpc.c:729:
+        if (calculate_geometry(total_sectors + i, &cyls, &heads,
+                               &secs_per_cyl))
+        {

The control flow statement may include more than one line.
Here it has two lines, and CODING_STYLE enforces my change.

I'll send a rebased version of my patch series as soon as
these and any open questions are solved.The rebased patches are
available from git://qemu.weilnetz.de/qemu.git.

Cheers,
Stefan

Patch

diff --git a/block/vpc.c b/block/vpc.c
index 7948609..1c55c21 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -136,13 +136,14 @@  typedef struct BDRVVPCState {
     Error *migration_blocker;
 } BDRVVPCState;
 
-static uint32_t vpc_checksum(uint8_t* buf, size_t size)
+static uint32_t vpc_checksum(uint8_t *buf, size_t size)
 {
     uint32_t res = 0;
     int i;
 
-    for (i = 0; i < size; i++)
+    for (i = 0; i < size; i++) {
         res += buf[i];
+    }
 
     return ~res;
 }
@@ -150,8 +151,9 @@  static uint32_t vpc_checksum(uint8_t* buf, size_t size)
 
 static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
-    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
-	return 100;
+    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
+        return 100;
+    }
     return 0;
 }
 
@@ -159,17 +161,18 @@  static int vpc_open(BlockDriverState *bs, int flags)
 {
     BDRVVPCState *s = bs->opaque;
     int i;
-    struct vhd_footer* footer;
-    struct vhd_dyndisk_header* dyndisk_header;
+    struct vhd_footer *footer;
+    struct vhd_dyndisk_header *dyndisk_header;
     uint8_t buf[HEADER_SIZE];
     uint32_t checksum;
     int err = -1;
     int disk_type = VHD_DYNAMIC;
 
-    if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
+    if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) {
         goto fail;
+    }
 
-    footer = (struct vhd_footer*) s->footer_buf;
+    footer = (struct vhd_footer *)s->footer_buf;
     if (strncmp(footer->creator, "conectix", 8)) {
         int64_t offset = bdrv_getlength(bs->file);
         if (offset < HEADER_SIZE) {
@@ -188,9 +191,10 @@  static int vpc_open(BlockDriverState *bs, int flags)
 
     checksum = be32_to_cpu(footer->checksum);
     footer->checksum = 0;
-    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum)
+    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) {
         fprintf(stderr, "block-vpc: The header checksum of '%s' is "
             "incorrect.\n", bs->filename);
+    }
 
     /* Write 'checksum' back to footer, or else will leave it with zero. */
     footer->checksum = be32_to_cpu(checksum);
@@ -293,8 +297,10 @@  static inline int64_t get_sector_offset(BlockDriverState *bs,
     pagetable_index = offset / s->block_size;
     pageentry_index = (offset % s->block_size) / 512;
 
-    if (pagetable_index >= s->max_table_entries || s->pagetable[pagetable_index] == 0xffffffff)
+    if (pagetable_index >= s->max_table_entries ||
+        s->pagetable[pagetable_index] == 0xffffffff) {
         return -1; // not allocated
+    }
 
     bitmap_offset = 512 * (uint64_t) s->pagetable[pagetable_index];
     block_offset = bitmap_offset + s->bitmap_size + (512 * pageentry_index);
@@ -312,35 +318,33 @@  static inline int64_t get_sector_offset(BlockDriverState *bs,
         bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
     }
 
-//    printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 ", bloff: %" PRIx64 "\n",
-//	sector_num, pagetable_index, pageentry_index,
-//	bitmap_offset, block_offset);
-
 // disabled by reason
 #if 0
 #ifdef CACHE
-    if (bitmap_offset != s->last_bitmap)
-    {
-	lseek(s->fd, bitmap_offset, SEEK_SET);
+    if (bitmap_offset != s->last_bitmap) {
+        lseek(s->fd, bitmap_offset, SEEK_SET);
 
-	s->last_bitmap = bitmap_offset;
+        s->last_bitmap = bitmap_offset;
 
-	// Scary! Bitmap is stored as big endian 32bit entries,
-	// while we used to look it up byte by byte
-	read(s->fd, s->pageentry_u8, 512);
-	for (i = 0; i < 128; i++)
-	    be32_to_cpus(&s->pageentry_u32[i]);
+        /* Scary! Bitmap is stored as big endian 32bit entries,
+         * while we used to look it up byte by byte */
+        read(s->fd, s->pageentry_u8, 512);
+        for (i = 0; i < 128; i++) {
+            be32_to_cpus(&s->pageentry_u32[i]);
+        }
     }
 
-    if ((s->pageentry_u8[pageentry_index / 8] >> (pageentry_index % 8)) & 1)
-	return -1;
+    if ((s->pageentry_u8[pageentry_index / 8] >> (pageentry_index % 8)) & 1) {
+        return -1;
+    }
 #else
     lseek(s->fd, bitmap_offset + (pageentry_index / 8), SEEK_SET);
 
     read(s->fd, &bitmap_entry, 1);
 
-    if ((bitmap_entry >> (pageentry_index % 8)) & 1)
-	return -1; // not allocated
+    if ((bitmap_entry >> (pageentry_index % 8)) & 1) {
+        return -1; /* not allocated */
+    }
 #endif
 #endif
 
@@ -353,15 +357,16 @@  static inline int64_t get_sector_offset(BlockDriverState *bs,
  *
  * Returns 0 on success and < 0 on error
  */
-static int rewrite_footer(BlockDriverState* bs)
+static int rewrite_footer(BlockDriverState *bs)
 {
     int ret;
     BDRVVPCState *s = bs->opaque;
     int64_t offset = s->free_data_block_offset;
 
     ret = bdrv_pwrite_sync(bs->file, offset, s->footer_buf, HEADER_SIZE);
-    if (ret < 0)
+    if (ret < 0) {
         return ret;
+    }
 
     return 0;
 }
@@ -373,7 +378,7 @@  static int rewrite_footer(BlockDriverState* bs)
  *
  * Returns the sectors' offset in the image file on success and < 0 on error
  */
-static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
+static int64_t alloc_block(BlockDriverState *bs, int64_t sector_num)
 {
     BDRVVPCState *s = bs->opaque;
     int64_t bat_offset;
@@ -382,13 +387,15 @@  static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
     uint8_t bitmap[s->bitmap_size];
 
     // Check if sector_num is valid
-    if ((sector_num < 0) || (sector_num > bs->total_sectors))
+    if ((sector_num < 0) || (sector_num > bs->total_sectors)) {
         return -1;
+    }
 
     // Write entry into in-memory BAT
     index = (sector_num * 512) / s->block_size;
-    if (s->pagetable[index] != 0xFFFFFFFF)
+    if (s->pagetable[index] != 0xFFFFFFFF) {
         return -1;
+    }
 
     s->pagetable[index] = s->free_data_block_offset / 512;
 
@@ -403,15 +410,17 @@  static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
     // Write new footer (the old one will be overwritten)
     s->free_data_block_offset += s->block_size + s->bitmap_size;
     ret = rewrite_footer(bs);
-    if (ret < 0)
+    if (ret < 0) {
         goto fail;
+    }
 
     // Write BAT entry to disk
     bat_offset = s->bat_offset + (4 * index);
     bat_value = be32_to_cpu(s->pagetable[index]);
     ret = bdrv_pwrite_sync(bs->file, bat_offset, &bat_value, 4);
-    if (ret < 0)
+    if (ret < 0) {
         goto fail;
+    }
 
     return get_sector_offset(bs, sector_num, 0);
 
@@ -492,8 +501,9 @@  static int vpc_write(BlockDriverState *bs, int64_t sector_num,
 
         if (offset == -1) {
             offset = alloc_block(bs, sector_num);
-            if (offset < 0)
+            if (offset < 0) {
                 return -1;
+            }
         }
 
         ret = bdrv_pwrite(bs->file, offset, buf, sectors * BDRV_SECTOR_SIZE);
@@ -532,8 +542,8 @@  static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
  * the hardware EIDE and ATA-2 limit of 16 heads (max disk size of 127 GB)
  * and instead allow up to 255 heads.
  */
-static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
-    uint8_t* heads, uint8_t* secs_per_cyl)
+static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
+    uint8_t *heads, uint8_t *secs_per_cyl)
 {
     uint32_t cyls_times_heads;
 
@@ -555,8 +565,9 @@  static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
         cyls_times_heads = total_sectors / *secs_per_cyl;
         *heads = (cyls_times_heads + 1023) / 1024;
 
-        if (*heads < 4)
+        if (*heads < 4) {
             *heads = 4;
+        }
 
         if (cyls_times_heads >= (*heads * 1024) || *heads > 16) {
             *secs_per_cyl = 31;
@@ -578,8 +589,8 @@  static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
 
 static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
 {
-    struct vhd_dyndisk_header* dyndisk_header =
-        (struct vhd_dyndisk_header*) buf;
+    struct vhd_dyndisk_header *dyndisk_header =
+        (struct vhd_dyndisk_header *)buf;
     size_t block_size, num_bat_entries;
     int i;
     int ret = -EIO;
@@ -709,8 +720,7 @@  static int vpc_create(const char *filename, QEMUOptionParameter *options)
     total_sectors = total_size / BDRV_SECTOR_SIZE;
     for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
         if (calculate_geometry(total_sectors + i, &cyls, &heads,
-                               &secs_per_cyl))
-        {
+                               &secs_per_cyl)) {
             ret = -EFBIG;
             goto fail;
         }