On Tue, 20 Jan 2015 13:33:27 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> Cornelia Huck <cornelia.h...@de.ibm.com> writes:
>
> > On Tue, 20 Jan 2015 10:45:41 +0100
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> This patch makes Coverity unhappy:
> >>
> >> *** CID 1264326: Unintended sign extension (SIGN_EXTENSION)
> >> /hw/s390x/s390-pci-inst.c: 787 in stpcifc_service_call()
> >> 781 stq_p(&fib.pal, pbdev->pal);
> >> 782 stq_p(&fib.iota, pbdev->g_iota);
> >> 783 stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr);
> >> 784 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr);
> >> 785 stq_p(&fib.fmb_addr, pbdev->fmb_addr);
> >> 786
> >> >>> CID 1264326: Unintended sign extension (SIGN_EXTENSION)
> >> >>> Suspicious implicit sign extension: "pbdev->isc" with type
> >> >>> "unsigned char" (8 bits, unsigned) is promoted in "(pbdev->isc <<
> >> >>> 28) | (pbdev->noi << 16)" to type "int" (32 bits, signed), then
> >> >>> sign-extended to type "unsigned long" (64 bits, unsigned). If
> >> >>> "(pbdev->isc << 28) | (pbdev->noi << 16)" is greater than
> >> >>> 0x7FFFFFFF, the upper bits of the result will all be 1.
> >> 787 data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> >> 788 (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> >> 789 pbdev->routes.adapter.summary_offset;
> >> 790 stw_p(&fib.data, data);
> >> 791
> >> 792 if (pbdev->fh >> ENABLE_BIT_OFFSET) {
> >
> > There's a fix for this (and the memory leak):
> >
> > http://marc.info/?l=qemu-devel&m=142124886620078&w=2
> >
> > The patch is sitting in my queue, will send with the next pile of s390x
> > updates.
>
> I can't see how
>
> @@ -787,7 +787,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1,
> uint64_t fiba)
> data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> pbdev->routes.adapter.summary_offset;
> - stw_p(&fib.data, data);
> + stl_p(&fib.data, data);
>
> if (pbdev->fh >> ENABLE_BIT_OFFSET) {
> fib.fc |= 0x80;
>
> fixes the implicit sign extension within the assignment preceding it.
What, I am expected to actually read the explanations? :)
> Regarding the leak, I prefer my patch, because it avoids the free on
> error. But you're the maintainer.
Indeed, that's a good point.
I'll drop Frank's original patch and instead take your memory leak fix.
Will take a patch from Frank for the sign extension stuff (and the
stw/stl fix) as well once it has been posted.
