[PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[~farzon]

From: Farzon Lotfi <hi@farzon.org>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

Signed-off-by: Farzon Lotfi <hi@farzon.org>
---
 block/bochs.c       | 10 +++++-----
 block/file-posix.c  |  8 ++++----
 block/file-win32.c  | 20 ++++++++++----------
 block/parallels.c   | 10 +++++-----
 block/qcow.c        | 10 +++++-----
 include/block/nbd.h |  2 +-
 6 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 2f010ab40a..01b84625c0 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -293,14 +293,14 @@ static void bochs_close(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_bochs = {
-    .format_name	= "bochs",
-    .instance_size	= sizeof(BDRVBochsState),
-    .bdrv_probe		= bochs_probe,
-    .bdrv_open		= bochs_open,
+    .format_name    = "bochs",
+    .instance_size  = sizeof(BDRVBochsState),
+    .bdrv_probe     = bochs_probe,
+    .bdrv_open      = bochs_open,
     .bdrv_child_perm     = bdrv_default_perms,
     .bdrv_refresh_limits = bochs_refresh_limits,
     .bdrv_co_preadv = bochs_co_preadv,
-    .bdrv_close		= bochs_close,
+    .bdrv_close     = bochs_close,
     .is_format          = true,
 };
 
diff --git a/block/file-posix.c b/block/file-posix.c
index d81e15efa4..9fc065506d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -127,7 +127,7 @@
 #define FTYPE_FILE   0
 #define FTYPE_CD     1
 
-#define MAX_BLOCKSIZE	4096
+#define MAX_BLOCKSIZE   4096
 
 /* Posix file locking bytes. Libvirt takes byte 0, we start from higher bytes,
  * leaving a few more bytes for its future use. */
@@ -3647,7 +3647,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
 
     .bdrv_co_truncate       = raw_co_truncate,
-    .bdrv_getlength	= raw_getlength,
+    .bdrv_getlength = raw_getlength,
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
@@ -3750,7 +3750,7 @@ static BlockDriver bdrv_host_cdrom = {
     .protocol_name      = "host_cdrom",
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_needs_filename = true,
-    .bdrv_probe_device	= cdrom_probe_device,
+    .bdrv_probe_device  = cdrom_probe_device,
     .bdrv_parse_filename = cdrom_parse_filename,
     .bdrv_file_open     = cdrom_open,
     .bdrv_close         = raw_close,
@@ -3881,7 +3881,7 @@ static BlockDriver bdrv_host_cdrom = {
     .protocol_name      = "host_cdrom",
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_needs_filename = true,
-    .bdrv_probe_device	= cdrom_probe_device,
+    .bdrv_probe_device  = cdrom_probe_device,
     .bdrv_parse_filename = cdrom_parse_filename,
     .bdrv_file_open     = cdrom_open,
     .bdrv_close         = raw_close,
diff --git a/block/file-win32.c b/block/file-win32.c
index b97c58d642..f80e62faf1 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -743,9 +743,9 @@ static QemuOptsList raw_create_opts = {
 };
 
 BlockDriver bdrv_file = {
-    .format_name	= "file",
-    .protocol_name	= "file",
-    .instance_size	= sizeof(BDRVRawState),
+    .format_name    = "file",
+    .protocol_name  = "file",
+    .instance_size  = sizeof(BDRVRawState),
     .bdrv_needs_filename = true,
     .bdrv_parse_filename = raw_parse_filename,
     .bdrv_file_open     = raw_open,
@@ -763,7 +763,7 @@ BlockDriver bdrv_file = {
     .bdrv_aio_flush     = raw_aio_flush,
 
     .bdrv_co_truncate   = raw_co_truncate,
-    .bdrv_getlength	= raw_getlength,
+    .bdrv_getlength = raw_getlength,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -915,14 +915,14 @@ done:
 }
 
 static BlockDriver bdrv_host_device = {
-    .format_name	= "host_device",
-    .protocol_name	= "host_device",
-    .instance_size	= sizeof(BDRVRawState),
+    .format_name    = "host_device",
+    .protocol_name  = "host_device",
+    .instance_size  = sizeof(BDRVRawState),
     .bdrv_needs_filename = true,
     .bdrv_parse_filename = hdev_parse_filename,
-    .bdrv_probe_device	= hdev_probe_device,
-    .bdrv_file_open	= hdev_open,
-    .bdrv_close		= raw_close,
+    .bdrv_probe_device  = hdev_probe_device,
+    .bdrv_file_open = hdev_open,
+    .bdrv_close     = raw_close,
     .bdrv_refresh_limits = hdev_refresh_limits,
 
     .bdrv_aio_preadv    = raw_aio_preadv,
diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bb..629d8aae2b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_parallels = {
-    .format_name	= "parallels",
-    .instance_size	= sizeof(BDRVParallelsState),
-    .bdrv_probe		= parallels_probe,
-    .bdrv_open		= parallels_open,
-    .bdrv_close		= parallels_close,
+    .format_name    = "parallels",
+    .instance_size  = sizeof(BDRVParallelsState),
+    .bdrv_probe     = parallels_probe,
+    .bdrv_open      = parallels_open,
+    .bdrv_close     = parallels_close,
     .bdrv_child_perm          = bdrv_default_perms,
     .bdrv_co_block_status     = parallels_co_block_status,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
diff --git a/block/qcow.c b/block/qcow.c
index f8919a44d1..f3327dbcae 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -1178,11 +1178,11 @@ static const char *const qcow_strong_runtime_opts[] = {
 };
 
 static BlockDriver bdrv_qcow = {
-    .format_name	= "qcow",
-    .instance_size	= sizeof(BDRVQcowState),
-    .bdrv_probe		= qcow_probe,
-    .bdrv_open		= qcow_open,
-    .bdrv_close		= qcow_close,
+    .format_name    = "qcow",
+    .instance_size  = sizeof(BDRVQcowState),
+    .bdrv_probe     = qcow_probe,
+    .bdrv_open      = qcow_open,
+    .bdrv_close     = qcow_close,
     .bdrv_child_perm        = bdrv_default_perms,
     .bdrv_reopen_prepare    = qcow_reopen_prepare,
     .bdrv_co_create         = qcow_co_create,
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 78d101b774..4a39955ee3 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -223,7 +223,7 @@ enum {
     NBD_CMD_BLOCK_STATUS = 7,
 };
 
-#define NBD_DEFAULT_PORT	10809
+#define NBD_DEFAULT_PORT    10809
 
 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
-- 
2.32.0

Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[Kevin Wolf]

Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
> From: Farzon Lotfi <hi@farzon.org>
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
> 
> Signed-off-by: Farzon Lotfi <hi@farzon.org>

Just picking one example, but it applies to most hunks of the patch:

> diff --git a/block/parallels.c b/block/parallels.c
> index 6ebad2a2bb..629d8aae2b 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
>  }
>  
>  static BlockDriver bdrv_parallels = {
> -    .format_name	= "parallels",
> -    .instance_size	= sizeof(BDRVParallelsState),
> -    .bdrv_probe		= parallels_probe,
> -    .bdrv_open		= parallels_open,
> -    .bdrv_close		= parallels_close,
> +    .format_name    = "parallels",
> +    .instance_size  = sizeof(BDRVParallelsState),
> +    .bdrv_probe     = parallels_probe,
> +    .bdrv_open      = parallels_open,
> +    .bdrv_close     = parallels_close,
>      .bdrv_child_perm          = bdrv_default_perms,
>      .bdrv_co_block_status     = parallels_co_block_status,
>      .bdrv_has_zero_init       = bdrv_has_zero_init_1,

When we're changing these lines anyway, let's make sure to have
consistent alignment with the surrounding code. So I would prefer
something like:

+    .bdrv_close               = parallels_close,
     .bdrv_child_perm          = bdrv_default_perms,

Rather than:

+    .bdrv_close     = parallels_close,
     .bdrv_child_perm          = bdrv_default_perms,

In most cases, there are already inconsistencies in the BlockDriver
definitions, but let's use the chance to make it a little better.

Kevin

Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[Vladimir Sementsov-Ogievskiy]

9/30/21 12:46, Kevin Wolf wrote:
> Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
>> From: Farzon Lotfi <hi@farzon.org>
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
>>
>> Signed-off-by: Farzon Lotfi <hi@farzon.org>
> 
> Just picking one example, but it applies to most hunks of the patch:
> 
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 6ebad2a2bb..629d8aae2b 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
>>   }
>>   
>>   static BlockDriver bdrv_parallels = {
>> -    .format_name	= "parallels",
>> -    .instance_size	= sizeof(BDRVParallelsState),
>> -    .bdrv_probe		= parallels_probe,
>> -    .bdrv_open		= parallels_open,
>> -    .bdrv_close		= parallels_close,
>> +    .format_name    = "parallels",
>> +    .instance_size  = sizeof(BDRVParallelsState),
>> +    .bdrv_probe     = parallels_probe,
>> +    .bdrv_open      = parallels_open,
>> +    .bdrv_close     = parallels_close,
>>       .bdrv_child_perm          = bdrv_default_perms,
>>       .bdrv_co_block_status     = parallels_co_block_status,
>>       .bdrv_has_zero_init       = bdrv_has_zero_init_1,
> 
> When we're changing these lines anyway, let's make sure to have
> consistent alignment with the surrounding code. So I would prefer
> something like:
> 
> +    .bdrv_close               = parallels_close,
>       .bdrv_child_perm          = bdrv_default_perms,
> 
> Rather than:
> 
> +    .bdrv_close     = parallels_close,
>       .bdrv_child_perm          = bdrv_default_perms,
> 
> In most cases, there are already inconsistencies in the BlockDriver
> definitions, but let's use the chance to make it a little better.


Or may be drop alignment around '=' at all, to have

    .bdrv_child_perm = bdrv_default_perms,
    .bdrv_co_block_status = parallels_co_block_status,
    .bdrv_has_zero_init = bdrv_has_zero_init_1,

?

This alignment in definitions is

1. source for alignment inconsistencies
2. infecting logic-changing patches by fixing surround alignment (or having to add separate patches to adjust old alignments, which is a real waste of time)
3. [1] and [2] will never be helpful for rebase conflicts resolution, when need to backport/forwardport commits.


and for that all we have a bit more readable definition, which is rarely read as is. (I think, it's more often used to navigate by tags, like bdrv_open -> jump to invocation in qcow2.c -> jump to qcow2_open)


-- 
Best regards,
Vladimir

Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[Kevin Wolf]

Am 30.09.2021 um 14:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 9/30/21 12:46, Kevin Wolf wrote:
> > Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
> > > From: Farzon Lotfi <hi@farzon.org>
> > > 
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
> > > 
> > > Signed-off-by: Farzon Lotfi <hi@farzon.org>
> > 
> > Just picking one example, but it applies to most hunks of the patch:
> > 
> > > diff --git a/block/parallels.c b/block/parallels.c
> > > index 6ebad2a2bb..629d8aae2b 100644
> > > --- a/block/parallels.c
> > > +++ b/block/parallels.c
> > > @@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
> > >   }
> > >   static BlockDriver bdrv_parallels = {
> > > -    .format_name	= "parallels",
> > > -    .instance_size	= sizeof(BDRVParallelsState),
> > > -    .bdrv_probe		= parallels_probe,
> > > -    .bdrv_open		= parallels_open,
> > > -    .bdrv_close		= parallels_close,
> > > +    .format_name    = "parallels",
> > > +    .instance_size  = sizeof(BDRVParallelsState),
> > > +    .bdrv_probe     = parallels_probe,
> > > +    .bdrv_open      = parallels_open,
> > > +    .bdrv_close     = parallels_close,
> > >       .bdrv_child_perm          = bdrv_default_perms,
> > >       .bdrv_co_block_status     = parallels_co_block_status,
> > >       .bdrv_has_zero_init       = bdrv_has_zero_init_1,
> > 
> > When we're changing these lines anyway, let's make sure to have
> > consistent alignment with the surrounding code. So I would prefer
> > something like:
> > 
> > +    .bdrv_close               = parallels_close,
> >       .bdrv_child_perm          = bdrv_default_perms,
> > 
> > Rather than:
> > 
> > +    .bdrv_close     = parallels_close,
> >       .bdrv_child_perm          = bdrv_default_perms,
> > 
> > In most cases, there are already inconsistencies in the BlockDriver
> > definitions, but let's use the chance to make it a little better.
> 
> 
> Or may be drop alignment around '=' at all, to have
> 
>    .bdrv_child_perm = bdrv_default_perms,
>    .bdrv_co_block_status = parallels_co_block_status,
>    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> 
> ?

You're right that this would make it easy to keep things consistent, but
I think it hurts readability a lot, even compared to the current, often
inconsistent state.

Kevin

Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[Eric Blake]

On Thu, Sep 30, 2021 at 02:55:16PM +0200, Kevin Wolf wrote:
> > > When we're changing these lines anyway, let's make sure to have
> > > consistent alignment with the surrounding code. So I would prefer
> > > something like:
> > > 
> > > +    .bdrv_close               = parallels_close,
> > >       .bdrv_child_perm          = bdrv_default_perms,
> > > 
> > > Rather than:
> > > 
> > > +    .bdrv_close     = parallels_close,
> > >       .bdrv_child_perm          = bdrv_default_perms,
> > > 
> > > In most cases, there are already inconsistencies in the BlockDriver
> > > definitions, but let's use the chance to make it a little better.
> > 
> > 
> > Or may be drop alignment around '=' at all, to have
> > 
> >    .bdrv_child_perm = bdrv_default_perms,
> >    .bdrv_co_block_status = parallels_co_block_status,
> >    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> > 
> > ?
> 
> You're right that this would make it easy to keep things consistent, but
> I think it hurts readability a lot, even compared to the current, often
> inconsistent state.

I agree that the alignment adds a bit of readability, but also
understand that it adds a maintenacne burden.  Thus, in code I manage,
I'm fine with either style (no extra spaces, or '=' lined up); and can
live with different styles in different files (which I then will honor
when doing a grunt-work patch that touches all of the block drivers).
But what I don't like is when a single file cannot be consistent with
itself on which of those two styles it is using - a file that uses
aligned '=' really needs to put that '=' far enough to the right that
an added long-named member doesn't cause frequent reindentation of the
rest of the members.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[Eric Blake]

On Wed, Sep 29, 2021 at 01:30:50AM -0400, ~farzon wrote:
> From: Farzon Lotfi <hi@farzon.org>

Food for thought: your git/mail configuration used one address for the
envelope (name '~farzon' as user 'farzon@') and another as the patch
author (name 'Farzon Lotfi' as user 'hi@').  Since you own your domain
(with its own perks), you can get away with it, but it looks a bit
less professional to need a second From: line to override the mail
author (which is more commonly needed to work around overly strict
DKIM settings), compared to just sending your mail from the desired
full-name author in the first place.  But since your Signed-off-by tag
is correct, this is not a show-stopper to applying your patch.

However, my next comment does require a respin before your patch would
be ready.  Your Subject: line is too long, as evidenced by your choice
of using sentences.  It should really be 'category: short description'
all within 60 characters or so (when 'git log' displays indentation, a
short commit id, and then your subject, you don't want your subject
truncated).  It feels like some of your subject should instead be part
of the commit body, where you currently have only...

> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

...because that one line as a body is rather sparse.  While the URL is
nice (it is a lifesaver for tracking whether a particular bug has a
known patch), it does not tell me at a glance what is behind that URL,
and I don't want to have to fire up my browser to learn about your
patch.  In general, the subject should be a short "what", and the
commit body should be "why" a maintainer should apply it.  I'd suggest
the following:

block: Replace TABs with space

Bring the block file in line with the QEMU coding style, with spaces
for indentation.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

> 
> Signed-off-by: Farzon Lotfi <hi@farzon.org>
> ---

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org