| Message ID | 20161216152319.12494-1-leif.lindholm@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH] smbios: stop ignoring command line options for TARGET_ARM
Message-id: 20161216152319.12494-1-leif.lindholm@linaro.org
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/148190196925.25504.12830466137601571123.stgit@bahia -> patchew/148190196925.25504.12830466137601571123.stgit@bahia
* [new tag] patchew/20161216152319.12494-1-leif.lindholm@linaro.org -> patchew/20161216152319.12494-1-leif.lindholm@linaro.org
Switched to a new branch 'test'
f9025dc smbios: stop ignoring command line options for TARGET_ARM
=== OUTPUT BEGIN ===
Checking PATCH 1/1: smbios: stop ignoring command line options for TARGET_ARM...
ERROR: do not initialise globals to 0 or NULL
#54: FILE: vl.c:162:
+int smbios_override = 0;
total: 1 errors, 0 warnings, 32 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
So, in addition to the style issue I was automatically notified of, I also neglected to CC the appropriate people - adding them here. As for the style issue - is it more important to adhere to checkpatch.pl or to surrounding definitions? Regards, Leif On Fri, Dec 16, 2016 at 03:23:19PM +0000, Leif Lindholm wrote: > Commit c30e1565 ("smbios: implement smbios support for mach-virt") > enabled automatic generation of SMBIOS tables for TARGET_ARM, and > actually provides data for the "virt" machine. > > However, do_smbios_option() still had an #ifdef TARGET_I386, preventing > any -smbios command line options from being parsed for any non-x86 > targets. > Change this to use a status variable instead of compile-time filtering. > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > > Verified on ARM mach-virt with UEFI shell "smbiosview" command and QEMU > command line parameter -smbios type=0,version=foobar. > > arch_init.c | 6 +++--- > include/hw/smbios/smbios.h | 2 ++ > vl.c | 2 ++ > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 5cc58b2..d4e28c0 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -250,9 +250,9 @@ void do_acpitable_option(const QemuOpts *opts) > > void do_smbios_option(QemuOpts *opts) > { > -#ifdef TARGET_I386 > - smbios_entry_add(opts); > -#endif > + if (smbios_override) { > + smbios_entry_add(opts); > + } > } > > int kvm_available(void) > diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h > index 1cd53cc..2a3dca2 100644 > --- a/include/hw/smbios/smbios.h > +++ b/include/hw/smbios/smbios.h > @@ -267,4 +267,6 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array, > const unsigned int mem_array_size, > uint8_t **tables, size_t *tables_len, > uint8_t **anchor, size_t *anchor_len); > + > +extern int smbios_override; > #endif /* QEMU_SMBIOS_H */ > diff --git a/vl.c b/vl.c > index d77dd86..8e71b06 100644 > --- a/vl.c > +++ b/vl.c > @@ -159,6 +159,7 @@ int smp_cpus = 1; > int max_cpus = 1; > int smp_cores = 1; > int smp_threads = 1; > +int smbios_override = 0; > int acpi_enabled = 1; > int no_hpet = 0; > int fd_bootchk = 1; > @@ -3711,6 +3712,7 @@ int main(int argc, char **argv, char **envp) > if (!opts) { > exit(1); > } > + smbios_override = 1; > do_smbios_option(opts); > break; > case QEMU_OPTION_fwcfg: > -- > 2.10.2 >
On Fri, 16 Dec 2016 15:23:19 +0000 Leif Lindholm <leif.lindholm@linaro.org> wrote: > Commit c30e1565 ("smbios: implement smbios support for mach-virt") > enabled automatic generation of SMBIOS tables for TARGET_ARM, and > actually provides data for the "virt" machine. > > However, do_smbios_option() still had an #ifdef TARGET_I386, preventing > any -smbios command line options from being parsed for any non-x86 > targets. > Change this to use a status variable instead of compile-time filtering. > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > > Verified on ARM mach-virt with UEFI shell "smbiosview" command and QEMU > command line parameter -smbios type=0,version=foobar. > > arch_init.c | 6 +++--- > include/hw/smbios/smbios.h | 2 ++ > vl.c | 2 ++ > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 5cc58b2..d4e28c0 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -250,9 +250,9 @@ void do_acpitable_option(const QemuOpts *opts) > > void do_smbios_option(QemuOpts *opts) > { > -#ifdef TARGET_I386 extending above condition to make it compiled for ARM would do what you need > - smbios_entry_add(opts); > -#endif > + if (smbios_override) { > + smbios_entry_add(opts); > + } > } > > int kvm_available(void) > diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h > index 1cd53cc..2a3dca2 100644 > --- a/include/hw/smbios/smbios.h > +++ b/include/hw/smbios/smbios.h > @@ -267,4 +267,6 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array, > const unsigned int mem_array_size, > uint8_t **tables, size_t *tables_len, > uint8_t **anchor, size_t *anchor_len); > + > +extern int smbios_override; Adding global variables generally is not welcomed and one should try to avoid it if it could be helped. > #endif /* QEMU_SMBIOS_H */ > diff --git a/vl.c b/vl.c > index d77dd86..8e71b06 100644 > --- a/vl.c > +++ b/vl.c > @@ -159,6 +159,7 @@ int smp_cpus = 1; > int max_cpus = 1; > int smp_cores = 1; > int smp_threads = 1; > +int smbios_override = 0; > int acpi_enabled = 1; > int no_hpet = 0; > int fd_bootchk = 1; > @@ -3711,6 +3712,7 @@ int main(int argc, char **argv, char **envp) > if (!opts) { > exit(1); > } > + smbios_override = 1; ^^^ practically turns follow up call into unconditional smbios_entry_add(opts) call, so what's the point for adding 'smbios_override' at all? Also this patch would break build for targets that don't link smbios.c (i.e. which don't have CONFIG_SMBIOS=y) > do_smbios_option(opts); > break; > case QEMU_OPTION_fwcfg:
On Wed, Dec 21, 2016 at 11:51:02AM +0100, Igor Mammedov wrote: > On Fri, 16 Dec 2016 15:23:19 +0000 > > Verified on ARM mach-virt with UEFI shell "smbiosview" command and QEMU > > command line parameter -smbios type=0,version=foobar. > > > > arch_init.c | 6 +++--- > > include/hw/smbios/smbios.h | 2 ++ > > vl.c | 2 ++ > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/arch_init.c b/arch_init.c > > index 5cc58b2..d4e28c0 100644 > > --- a/arch_init.c > > +++ b/arch_init.c > > @@ -250,9 +250,9 @@ void do_acpitable_option(const QemuOpts *opts) > > > > void do_smbios_option(QemuOpts *opts) > > { > > -#ifdef TARGET_I386 > extending above condition to make it compiled for ARM > would do what you need Yes, but given how tedious that was to track down without a decent grasp of the source tree structure I had hoped to improve the experience for future newcomers. If that's not considered important, sure, I could hack together a v2 consisting only of that. > > - smbios_entry_add(opts); > > -#endif > > + if (smbios_override) { > > + smbios_entry_add(opts); > > + } > > } > > > > int kvm_available(void) > > diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h > > index 1cd53cc..2a3dca2 100644 > > --- a/include/hw/smbios/smbios.h > > +++ b/include/hw/smbios/smbios.h > > @@ -267,4 +267,6 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array, > > const unsigned int mem_array_size, > > uint8_t **tables, size_t *tables_len, > > uint8_t **anchor, size_t *anchor_len); > > + > > +extern int smbios_override; > Adding global variables generally is not welcomed and one > should try to avoid it if it could be helped. That is certainly a fair comment. Not being too familiar with the codebase I was slavishly trying to follow the style of the surrounding code. Will avoid that in future. > > #endif /* QEMU_SMBIOS_H */ > > diff --git a/vl.c b/vl.c > > index d77dd86..8e71b06 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -159,6 +159,7 @@ int smp_cpus = 1; > > int max_cpus = 1; > > int smp_cores = 1; > > int smp_threads = 1; > > +int smbios_override = 0; > > int acpi_enabled = 1; > > int no_hpet = 0; > > int fd_bootchk = 1; > > @@ -3711,6 +3712,7 @@ int main(int argc, char **argv, char **envp) > > if (!opts) { > > exit(1); > > } > > + smbios_override = 1; > ^^^ practically turns follow up call into unconditional > smbios_entry_add(opts) call, Which is what it already is for TARGET_I386. > so what's the point for adding 'smbios_override' at all? Apparently a misunderstanding of the underlying command line handling mechanics. > Also this patch would break build for targets that don't link smbios.c > (i.e. which don't have CONFIG_SMBIOS=y) Ah, I hadn't spotted that - apologies. So a simpler, and more correct fix would rather be to change the #ifdef TARGET_I386 in arch_init.c to #ifdef CONFIG_SMBIOS ? ... if there had been a conveniently available CONFIG_SMBIOS. Would it be acceptable to add one to config-target.h or is that reserved for host-specific options? Regards, Leif > > do_smbios_option(opts); > > break; > > case QEMU_OPTION_fwcfg: >
On Wed, 21 Dec 2016 12:35:09 +0000 Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Wed, Dec 21, 2016 at 11:51:02AM +0100, Igor Mammedov wrote: > > On Fri, 16 Dec 2016 15:23:19 +0000 > > > Verified on ARM mach-virt with UEFI shell "smbiosview" command and QEMU > > > command line parameter -smbios type=0,version=foobar. > > > > > > arch_init.c | 6 +++--- > > > include/hw/smbios/smbios.h | 2 ++ > > > vl.c | 2 ++ > > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch_init.c b/arch_init.c > > > index 5cc58b2..d4e28c0 100644 > > > --- a/arch_init.c > > > +++ b/arch_init.c > > > @@ -250,9 +250,9 @@ void do_acpitable_option(const QemuOpts *opts) > > > > > > void do_smbios_option(QemuOpts *opts) > > > { > > > -#ifdef TARGET_I386 > > extending above condition to make it compiled for ARM > > would do what you need > > Yes, but given how tedious that was to track down without a decent > grasp of the source tree structure I had hoped to improve the > experience for future newcomers. > > If that's not considered important, sure, I could hack together a v2 > consisting only of that. > > > > - smbios_entry_add(opts); > > > -#endif > > > + if (smbios_override) { > > > + smbios_entry_add(opts); > > > + } > > > } > > > > > > int kvm_available(void) > > > diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h > > > index 1cd53cc..2a3dca2 100644 > > > --- a/include/hw/smbios/smbios.h > > > +++ b/include/hw/smbios/smbios.h > > > @@ -267,4 +267,6 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array, > > > const unsigned int mem_array_size, > > > uint8_t **tables, size_t *tables_len, > > > uint8_t **anchor, size_t *anchor_len); > > > + > > > +extern int smbios_override; > > Adding global variables generally is not welcomed and one > > should try to avoid it if it could be helped. > > That is certainly a fair comment. Not being too familiar with the > codebase I was slavishly trying to follow the style of the surrounding > code. Will avoid that in future. > > > > #endif /* QEMU_SMBIOS_H */ > > > diff --git a/vl.c b/vl.c > > > index d77dd86..8e71b06 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -159,6 +159,7 @@ int smp_cpus = 1; > > > int max_cpus = 1; > > > int smp_cores = 1; > > > int smp_threads = 1; > > > +int smbios_override = 0; > > > int acpi_enabled = 1; > > > int no_hpet = 0; > > > int fd_bootchk = 1; > > > @@ -3711,6 +3712,7 @@ int main(int argc, char **argv, char **envp) > > > if (!opts) { > > > exit(1); > > > } > > > + smbios_override = 1; > > ^^^ practically turns follow up call into unconditional > > smbios_entry_add(opts) call, > > Which is what it already is for TARGET_I386. > > > so what's the point for adding 'smbios_override' at all? > > Apparently a misunderstanding of the underlying command line handling > mechanics. > > > Also this patch would break build for targets that don't link smbios.c > > (i.e. which don't have CONFIG_SMBIOS=y) > > Ah, I hadn't spotted that - apologies. Just do 1 build for all targets before posting patches to avoid such kind of errors. > > So a simpler, and more correct fix would rather be to change the > #ifdef TARGET_I386 > in arch_init.c to > #ifdef CONFIG_SMBIOS it looks better to me than enumerating targets explicitly, CCing Paolo for another opinion > ... if there had been a conveniently available CONFIG_SMBIOS. > Would it be acceptable to add one to config-target.h or is that > reserved for host-specific options? > > Regards, > > Leif > > > > do_smbios_option(opts); > > > break; > > > case QEMU_OPTION_fwcfg: > > >
On 21/12/2016 14:59, Igor Mammedov wrote: >> Apparently a misunderstanding of the underlying command line handling >> mechanics. >> >>> Also this patch would break build for targets that don't link smbios.c >>> (i.e. which don't have CONFIG_SMBIOS=y) >> >> Ah, I hadn't spotted that - apologies. > > Just do 1 build for all targets before posting patches to avoid > such kind of errors. > >> So a simpler, and more correct fix would rather be to change the >> #ifdef TARGET_I386 >> in arch_init.c to >> #ifdef CONFIG_SMBIOS > > it looks better to me than enumerating targets explicitly, > CCing Paolo for another opinion I don't think CONFIG_SMBIOS is visible from C, is it? However, the solution is to: 1) add a smbios-stub.c file to hw/smbios, containing a dummy implementation of smbios_entry_add. For the Makefile magic see hw/pci/Makefile.objs. 2) add an Error * argument to smbios_entry_add, and make the stub version fail 3) remove do_smbios_option altogether, and make vl.c call smbios_entry_add directly. Paolo
On Wed, Dec 21, 2016 at 06:58:44PM +0100, Paolo Bonzini wrote: > On 21/12/2016 14:59, Igor Mammedov wrote: > >> Apparently a misunderstanding of the underlying command line handling > >> mechanics. > >> > >>> Also this patch would break build for targets that don't link smbios.c > >>> (i.e. which don't have CONFIG_SMBIOS=y) > >> > >> Ah, I hadn't spotted that - apologies. > > > > Just do 1 build for all targets before posting patches to avoid > > such kind of errors. > > > >> So a simpler, and more correct fix would rather be to change the > >> #ifdef TARGET_I386 > >> in arch_init.c to > >> #ifdef CONFIG_SMBIOS > > > > it looks better to me than enumerating targets explicitly, > > CCing Paolo for another opinion > > I don't think CONFIG_SMBIOS is visible from C, is it? No. (A bit of context that was drop asked: "... if there had been a conveniently available CONFIG_SMBIOS. Would it be acceptable to add one to config-target.h or is that reserved for host-specific options?" But that doesn't matter now.) > However, the solution is to: > > 1) add a smbios-stub.c file to hw/smbios, containing a dummy > implementation of smbios_entry_add. For the Makefile magic see > hw/pci/Makefile.objs. > > 2) add an Error * argument to smbios_entry_add, and make the stub > version fail > > 3) remove do_smbios_option altogether, and make vl.c call > smbios_entry_add directly. Many thanks for the pointers, I've done just that and am sending it out (as a new patch rather than a v2 because ... it bears no resemblance to the first one). Regards, Leif
diff --git a/arch_init.c b/arch_init.c index 5cc58b2..d4e28c0 100644 --- a/arch_init.c +++ b/arch_init.c @@ -250,9 +250,9 @@ void do_acpitable_option(const QemuOpts *opts) void do_smbios_option(QemuOpts *opts) { -#ifdef TARGET_I386 - smbios_entry_add(opts); -#endif + if (smbios_override) { + smbios_entry_add(opts); + } } int kvm_available(void) diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h index 1cd53cc..2a3dca2 100644 --- a/include/hw/smbios/smbios.h +++ b/include/hw/smbios/smbios.h @@ -267,4 +267,6 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array, const unsigned int mem_array_size, uint8_t **tables, size_t *tables_len, uint8_t **anchor, size_t *anchor_len); + +extern int smbios_override; #endif /* QEMU_SMBIOS_H */ diff --git a/vl.c b/vl.c index d77dd86..8e71b06 100644 --- a/vl.c +++ b/vl.c @@ -159,6 +159,7 @@ int smp_cpus = 1; int max_cpus = 1; int smp_cores = 1; int smp_threads = 1; +int smbios_override = 0; int acpi_enabled = 1; int no_hpet = 0; int fd_bootchk = 1; @@ -3711,6 +3712,7 @@ int main(int argc, char **argv, char **envp) if (!opts) { exit(1); } + smbios_override = 1; do_smbios_option(opts); break; case QEMU_OPTION_fwcfg:
Commit c30e1565 ("smbios: implement smbios support for mach-virt") enabled automatic generation of SMBIOS tables for TARGET_ARM, and actually provides data for the "virt" machine. However, do_smbios_option() still had an #ifdef TARGET_I386, preventing any -smbios command line options from being parsed for any non-x86 targets. Change this to use a status variable instead of compile-time filtering. Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> --- Verified on ARM mach-virt with UEFI shell "smbiosview" command and QEMU command line parameter -smbios type=0,version=foobar. arch_init.c | 6 +++--- include/hw/smbios/smbios.h | 2 ++ vl.c | 2 ++ 3 files changed, 7 insertions(+), 3 deletions(-)