Re: [Qemu-devel] [v2 PATCH 00/13] SMBIOS: build full tables in QEMU

[Kevin O'Connor]

On Tue, Mar 11, 2014 at 11:16:16AM -0400, Gabriel L. Somlo wrote:
> On Tue, Mar 11, 2014 at 11:03:06AM +0100, Gerd Hoffmann wrote:
> > Issue #1: There are checkpatch errors (scripts/checkpatch.pl).
> 
> I fixed most of those, with one exception (patch #5):
> 
> ERROR: do not use assignment in if condition
> #139: FILE: hw/i386/smbios.c:253:
> +        if (value != NULL && (len = strlen(value) + 1) > 1) { \
> 
> 'value' can be NULL, "", or "foo". The whole block looks like this:
> 
>   int len;
>   if (value != NULL && (len = strlen(value)) > 0) {

I wont comment on the value of checkpatch, but you could write it this
way if you wanted:

int len = value ? strlen(value) : 0;
if (len > 0) {

[...]
> > -Handle 0x1100, DMI type 17, 21 bytes
> > +Handle 0x1100, DMI type 17, 27 bytes
> >  Memory Device
> >     Array Handle: 0x1000
> > -   Error Information Handle: 0x0003
> > +   Error Information Handle: Not Provided
> 
> AFAICT, SeaBIOS doesn't set the error info handle at all, and you simply
> get whatever garbage happens to be contained in that memory location at
> the time. Typically it's 0x0000, but you happened to get 0x0003 :) I
> figured I'd set it to a known value (0xFFFE or "n/a").

That's a seabios bug - I'll create a seabios patch.

> On Tue, Mar 11, 2014 at 09:27:31AM -0400, Kevin O'Connor wrote:
> > I think it would be best to get the patch series to the point that
> > there is no diff between old and new.  That will make review easier,
> > and subsequent patches can then add new features.
> 
> Modulo the error info handle on type 17, and the fact that QEMU's version
> of type 17 has v2.3 fields and SeaBIOS's does not (kinda the whole reason
> I'm doing this in the first place :), that should be possible by just
> tweaking the defaults in the new smbios_set_defaults() function. I just
> feel weird setting "Bochs" as the default manufacturer... 

I would suggest being "bug for bug" compatible in the first set of
patches, and then add patches on top to add the additional
functionality.  Just my 2 cents.

-Kevin