Re: [Qemu-devel] [PATCH v8 02/24] hw/arm/virt: Move common definitions to virt.h

[Laszlo Ersek <lersek@redhat.com>]

On 05/21/15 10:25, Igor Mammedov wrote:
> On Thu, 21 May 2015 10:28:29 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> 
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Move some common definitions to virt.h. These will be used by
>> generating ACPI tables.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/arm/virt.c         | 21 +------------------
>>  include/hw/arm/virt.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 57 insertions(+), 20 deletions(-)
>>  create mode 100644 include/hw/arm/virt.h
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index a7f9a10..8959d0c 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/sysbus.h"
>>  #include "hw/arm/arm.h"
>>  #include "hw/arm/primecell.h"
>> +#include "hw/arm/virt.h"
>>  #include "hw/devices.h"
>>  #include "net/net.h"
>>  #include "sysemu/block-backend.h"
>> @@ -44,8 +45,6 @@
>>  #include "qemu/error-report.h"
>>  #include "hw/pci-host/gpex.h"
>>  
>> -#define NUM_VIRTIO_TRANSPORTS 32
>> -
>>  /* Number of external interrupt lines to configure the GIC with */
>>  #define NUM_IRQS 128
>>  
>> @@ -60,24 +59,6 @@
>>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>>  
>> -enum {
>> -    VIRT_FLASH,
>> -    VIRT_MEM,
>> -    VIRT_CPUPERIPHS,
>> -    VIRT_GIC_DIST,
>> -    VIRT_GIC_CPU,
>> -    VIRT_UART,
>> -    VIRT_MMIO,
>> -    VIRT_RTC,
>> -    VIRT_FW_CFG,
>> -    VIRT_PCIE,
>> -};
>> -
>> -typedef struct MemMapEntry {
>> -    hwaddr base;
>> -    hwaddr size;
>> -} MemMapEntry;
>> -
>>  typedef struct VirtBoardInfo {
>>      struct arm_boot_info bootinfo;
>>      const char *cpu_model;
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> new file mode 100644
>> index 0000000..2fe0d2e
>> --- /dev/null
>> +++ b/include/hw/arm/virt.h
>> @@ -0,0 +1,56 @@
>> +/*
>> + *
>> + * Copyright (c) 2015 Linaro Limited
> out of curiosity, git blame - tells that moved code was authored by:
> Laszlo Ersek       
> Alexander Graf         
> Peter Maydell     
> shouldn't they be mentioned here as authors?

Since Shannon is doing this in Linaro colors, and the notice on the
original file is also (C) Linaro, I think the patch should be OK in this
regard. If we wanted to be very pedantic, then the years could be
unified / extended as

  Copyright (c) 2013, 2015 Linaro Limited

because the original states 2013.

--o--

Instead of codifying the authors of the original file -- from "git
blame" -- in the *copy*, you could argue that some of the authors of the
*original* file shouldn't have been lazy :), and should have added their
own notices whenever they modified the original file. Then Shannon's
copy here would be up to date immediately.

However, I certainly don't add the RH copyright notice to every file I
touch.

When I add a new file (which requires a new license block):
- if it's a brand new file, I add the RH notice,
- if it's a (partial / customized) copy, I keep the original notices,
  and add our own.

But polluting every preexistent file with someone's own (C) notice, no
matter how small or trivial the change is, is just untenable in a
distributed development scenario.

(Independently, that's exactly what Intel do in the edk2 code base -- no
matter how trivial or insignificant the change, an Intel contributor
*always* adds their copyright notice (or at least updates the year
numbers) in the affected, preexistent file.

The churn it creates is *incredibly* pompous and annoying.)

So yes, after this patch, the world might not know *immediately* that
Red Hat hold copyright over the VIRT_FW_CFG enumeration constant in
"include/hw/arm/virt.h" (*), originating from commit 578f3c7b, because
the license block at the top doesn't spell it out, and because on a new
file (that is not the result of a rename) "git blame" won't help *directly*.

((*) I hope you feel the hilarity of this statement)

Nonetheless, the line in question will be possible to track back to this
patch, and when this commit-to-be will be shown fully, then the other
file, "hw/arm/virt.c", will enter the scope as well, and "git blame" on
*that* file will show the right authorship.

So, locating authorship and copyright works precisely the same as
assigning blame for a bug -- repeated use of "git blame" and "git show".

Anally maintaining copyright notices at the top, in preexistent files,
in this day and age, is just outdated. (I'm not annoyed at your
suggestion -- I'm annoyed by my memories of all those hunks in the Intel
edk2 commits. Shudder.)

TL;DR: don't bother.

Thanks
Laszlo

> 
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2 or later, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + * Emulate a virtual board which works by passing Linux all the information
>> + * it needs about what devices are present via the device tree.
>> + * There are some restrictions about what we can do here:
>> + *  + we can only present devices whose Linux drivers will work based
>> + *    purely on the device tree with no platform data at all
>> + *  + we want to present a very stripped-down minimalist platform,
>> + *    both because this reduces the security attack surface from the guest
>> + *    and also because it reduces our exposure to being broken when
>> + *    the kernel updates its device tree bindings and requires further
>> + *    information in a device binding that we aren't providing.
>> + * This is essentially the same approach kvmtool uses.
>> + */
>> +
>> +#ifndef QEMU_ARM_VIRT_H
>> +#define QEMU_ARM_VIRT_H
>> +
>> +#include "qemu-common.h"
>> +
>> +#define NUM_VIRTIO_TRANSPORTS 32
>> +
>> +enum {
>> +    VIRT_FLASH,
>> +    VIRT_MEM,
>> +    VIRT_CPUPERIPHS,
>> +    VIRT_GIC_DIST,
>> +    VIRT_GIC_CPU,
>> +    VIRT_UART,
>> +    VIRT_MMIO,
>> +    VIRT_RTC,
>> +    VIRT_FW_CFG,
>> +    VIRT_PCIE,
>> +};
>> +
>> +typedef struct MemMapEntry {
>> +    hwaddr base;
>> +    hwaddr size;
>> +} MemMapEntry;
>> +
>> +
>> +#endif
>