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
