Re: [Xen-devel] [PATCH v5 03/21] acpi: Prevent GPL-only code from seeping into non-GPL binaries

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/23/2016 05:18 AM, Jan Beulich wrote:
>
>> @@ -36,18 +37,25 @@ ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h: %.h: %.asl iasl
>>  mk_dsdt: mk_dsdt.c
>>      $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -o $@ mk_dsdt.c
>>  
>> -dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl mk_dsdt
>> +ifeq ($(GPL),y)
>> +dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl gpl/mk_dsdt_gpl.sh 
>> mk_dsdt
>>      awk 'NR > 1 {print s} {s=$$0}' $< > $@.$(TMP_SUFFIX)
>> +    # Strip license comment
>> +    sed -i '1,/\*\//{/\/\*/,/\*\//d}' $@.$(TMP_SUFFIX)
>> +    ./gpl/mk_dsdt_gpl.sh >> $@.$(TMP_SUFFIX)
> I don't think the leading ./ is necessary here, ...
>
>>      cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
>>      ./mk_dsdt --debug=$(debug) --dm-version qemu-xen >> $@.$(TMP_SUFFIX)
> ... other than e.g. here.

What is the difference between the two?

>
> Also I think shell scripts get better invoked via $(SHELL), to avoid
> running into problems when on some file system they end up without
> execute permission.
>
>> --- /dev/null
>> +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh
>> @@ -0,0 +1,110 @@
>> +# 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, 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/>.
>> +#
>> +
>> +printf "\n    /* Beginning of GPL-only code */\n\n"
>> +
>> +printf "    /* _S3 and _S4 are in separate SSDTs */\n"
>> +printf "    Name (\_S5, Package (0x04) {\n"
>> +printf "        0x00,  /* PM1a_CNT.SLP_TYP */\n"
>> +printf "        0x00,  /* PM1b_CNT.SLP_TYP */\n"
>> +printf "        0x00,  /* reserved */\n"
>> +printf "        0x00   /* reserved */\n"
>> +printf "    })\n"
>> +
>> +printf "    Name(PICD, 0)\n"
>> +printf "    Method(_PIC, 1) {\n"
>> +printf "        Store(Arg0, PICD)\n"
>> +printf "    }\n"
> Wouldn't this be better readable with "echo", avoiding all the \n
> instances? Actually this seems to even apply to most if not
> everything further down, as so far I didn't spot a case where
> you actually pass anything other than just a format string.

Format string is the only reason. There are a couple of instances where
output is formatted and I felt that having both echo and printf would
feel inconsistent.

>
>> +# PCI-ISA link definitions
>> +
>> +# BUFA: List of ISA IRQs available for linking to PCI INTx.
>> +printf "    Scope ( \_SB.PCI0 )  {\n"
>> +printf "        Name ( BUFA, ResourceTemplate() { IRQ(Level, ActiveLow, 
>> Shared) { 5, 10, 11 } } )\n"
>> +# BUFB: IRQ descriptor for returning from link-device _CRS methods.
>> +printf "        Name ( BUFB, Buffer() { 0x23, 0x00, 0x00, 0x18, 0x79, 0 } 
>> )\n"
>> +printf "        CreateWordField ( BUFB, 0x01, IRQV )\n"
>> +
>> +links=(A B C D)
> Is this portable?

I heard that associative arrays sometimes have portability issues but
this is a regular one. FWIW this runs on a 2009 Fedora12.

Would 'declare -a links' help?

>
>> +for i in `seq 0 3`;
>> +do
> I think you want to use only one of ; and newline as separator here.
> And is there a reason to prefer backquotes over the imo better
> legible $()?

No reason, that's the style I usually use. I can change to $() if that's
the preferred one.

(I will resend this patch only to avoid spamming everyone with 20
already-acked patches)

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel