Re: MP factor table

On Monday, June 22, 2009 Fish wrote:
> Proposed patch:
...
> - -                memcpy(sysib122->mpfact,mpfact,(MAX_CPU-1)*2);
> +                // memcpy(sysib122->mpfact,mpfact,(MAX_CPU-1)*2);
> +                get_mpfactors((BYTE*)sysib122->mpfact);
>                  STORE_FW(sysib122->accap, STSI_CAPABILITY);
> - -                memcpy(sysib122->ampfact,mpfact,(MAX_CPU-1)*2);
> +                //memcpy(sysib122->ampfact,mpfact,(MAX_CPU-1)*2);
> +                get_mpfactors((BYTE*)sysib122->ampfact);

When replacing statements, don't leave the old statement as a comment.
Subversion does a neater job of tracking changes and allowing them to
be reverted if necessary.


> +    /* APAR OA20135:
> +
> +       SUPPORT THE NEW MP FACTOR RANGE (0-65535) OF Z10 SERVERS
> +
> +       http://www-01.ibm.com/support/docview.wss?uid=isg1OA20135
> +

Do not include text of an IBM apar in the code. It could be regarded
as a copyright infringement, and at best it looks like plagiarism. If
necessary, you could simply refer to the apar number. But in this case
I don't think it's necessary since all of the information is in the
principles of operation manual.

> +            mpfactors[i] = CSWAP16( (U16) mpfactor );

To store a U16 into main storage in S/370 format, use STORE_HW not CSWAP16.

> +    /* Return the requested information... */
> +    memcpy( dest, &mpfactors[0], (MAX_CPU-1) * sizeof(U16) );

Functions which store into caller's storage without allowing the
caller to specify the maximum buffer length are considered to be a
potential security flaw (e.g. http://en.wikipedia.org/wiki/Gets)

All in all, I think this patch is over-engineered. It's fixing two
problems: (1) the current mp factors are wrong, and (2) the values are
inconsistent between STSI and SERVC. It would be sufficient just to
correct the values in the static table and have the table shared
between STSI and SERVC.

http://en.wikipedia.org/wiki/Occam%27s_Razor

P.S. Code changes are normally reviewed on the zHercules list.

--
Regards,
Roger Bowler
http://perso.wanadoo.fr/rbowler
Hercules "I can't believe it's not a mainframe!"