MirAL

Merge lp://qastaging/~alan-griffiths/miral/fix-1717061 into lp://qastaging/miral

fix-1717061
Merge into trunk

Proposed by Alan Griffiths on 2017-09-15

Status:

Merged

Approved by:

Alan Griffiths on 2017-09-20

Approved revision:

580

Merged at revision:

580

Proposed branch:

lp://qastaging/~alan-griffiths/miral/fix-1717061

Merge into:

lp://qastaging/miral

Diff against target:

350 lines (+174/-39)

7 files modified

miral/CMakeLists.txt (+1/-0)
miral/set_window_management_policy.cpp (+16/-16)
miral/window_info.cpp (+49/-15)
miral/window_info_defaults.h (+36/-0)
miral/window_management_trace.cpp (+10/-8)
test/CMakeLists.txt (+1/-0)
test/window_info.cpp (+61/-0)

To merge this branch:

bzr merge lp://qastaging/~alan-griffiths/miral/fix-1717061

Critical

Fix Committed

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2017-09-20
Gerry Boland (community)		2017-09-15	Approve on 2017-09-18
Brandon Schaefer (community)			Approve on 2017-09-15
Review via email: mp+330832@code.qastaging.launchpad.net

Commit message

Define default window settings in one place and clamp the actual values to avoid ldiv0. (LP: #1717061)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-15:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/miral-ci/76/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-15:

FAILED: Continuous integration, rev:580
https://mir-jenkins.ubuntu.com/job/miral-ci/77/
Executed test runs:
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-miral/123/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5243
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5231
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5231
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5231

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/miral-ci/77/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-09-15:

+unsigned clamp_dim(unsigned dim)
+{
+ return std::min<unsigned long>(std::numeric_limits<long>::max(), dim);
+}

There's a mix of types here: unsigned int, unsigned long and signed long, which is causing me problems understanding. I know sizeof(unsigned int) <= sizeof(long), which implies to me that

dim <= std::numeric_limits<unsigned>::max()
<= std::numeric_limits<long>::max()

so the min() will always choose "dim". So I don't understand what it is clamping to.

+// For our convenience when doing calculations we clamp the dimensions of the aspect ratio
+// so they will fit into a long without overflow.
What "long"? AspectRatio is a pair of unsigned ints.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-09-15:

> +unsigned clamp_dim(unsigned dim)
> +{
> + return std::min<unsigned long>(std::numeric_limits<long>::max(), dim);
> +}
>
> There's a mix of types here: unsigned int, unsigned long and signed long,
> which is causing me problems understanding. I know sizeof(unsigned int) <=
> sizeof(long), which implies to me that
>
> dim <= std::numeric_limits<unsigned>::max()
> <= std::numeric_limits<long>::max()

when, as Brandon encountered on armhf, long and int are the same size.

std::numeric_limits<long>::max() <= std::numeric_limits<unsigned>::max()

> so the min() will always choose "dim". So I don't understand what it is
> clamping to.

In the above case it is clamping to std::numeric_limits<long>::max()

> +// For our convenience when doing calculations we clamp the dimensions of the
> aspect ratio
> +// so they will fit into a long without overflow.
> What "long"? AspectRatio is a pair of unsigned ints.

If std::numeric_limits<long>::max() <= dim <= std::numeric_limits<unsigned>::max() then dim doesn't fit in a long.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2017-09-15:

Yeah this seems a bit more robust then what I was doing in my branch.

I was just figuring since sizeof(long) >= sizeof(int) we could just cheat and assign the aspect to int::max() vs unsigned::max() but you do end up losing a byte there if really wanted on amd64 machines.

Though wont it all be going through the values that are depended in the RPC? Should we at some point look at moving to proper int sizes (u)int8/16/32/64 to know we wont have changing int values depending on the architecture.

Also that test is sort of ... different then this overflow issue but still nice to test. Wasnt sure if we actually had more checking higher up the stack when we come from the RPC.

LGTM

review: Approve

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-09-18:

lgtm

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-18:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) on 2017-09-20:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Mir development team

MirAL

Merge lp://qastaging/~alan-griffiths/miral/fix-1717061 into lp://qastaging/miral

Commit message

Description of the change

Preview Diff

Subscribers