xpcom/io/nsLocalFileUnix.cpp does not build on linux with musl libc (BLOCK_SIZE undeclared)

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: felix.janda, Assigned: felix.janda)

Tracking

Trunk
mozilla41
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

4 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
Building xpcom on linux with musl libc (http://musl-libc.org) fails in xpcom/io/nsLocalFileUnix.cpp because on musl sys/quota.h does not define BLOCK_SIZE. (http://git.musl-libc.org/cgit/musl/tree/include/sys/quota.h)

BLOCK_SIZE is defined by the linux/fs.h, 1024 on all architectures and cannot be changed without breaking user space.

The attached patch makes BLOCK_SIZE explicit in the two places where it is used.
Attachment #8589471 - Flags: review?(nfroyd)
Comment on attachment 8589471 [details] [diff] [review]
Proposed patch

Review of attachment 8589471 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for pointing out the problem and supplying a patch.

Judging by:

gcc -E -dM -include sys/quota.h -x c /dev/null -o - |grep BLOCK_SIZE
gcc -E -dM -include sys/mount.h -x c /dev/null -o - |grep BLOCK_SIZE

it looks like it's actually <sys/mount.h> which defines BLOCK_SIZE, and musl's version doesn't define BLOCK_SIZE:

http://git.musl-libc.org/cgit/musl/tree/include/sys/mount.h

I'm not clear on whether this is a bug in musl or not; it looks like musl's <sys/mount.h> already copies several constants from <linux/fs.h>.  It looks like nothing in musl defines BLOCK_SIZE, though...

Anyway, the better way to fix this is to add something like:

// Some libcs don't define BLOCK_SIZE in <sys/mount.h>.
#ifndef BLOCK_SIZE
#define BLOCK_SIZE 1024
#endif

after the #includes in this block:

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#27
Attachment #8589471 - Flags: review?(nfroyd)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 2

4 years ago
Thanks for your input.

First, I've found the relevant previous bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=918140
Note that I would have nothing to report if the patch https://bugzilla.mozilla.org/attachment.cgi?id=8522932 had been committed. I also see that my patch should also remove the <sys/mount.h> include. Should I ask someone from this bug report for input?

I think (I can also ask) that no BLOCK_SIZE in sys/mount.h is a feature. BLOCK_SIZE is a fairly generic name which might collide with other code ("name space pollution") and I don't see it documented. For example http://git.busybox.net/busybox/tree/util-linux/minix.h?id=606291beabab14c85a141c7a4225fbcab8d19fbd#n54 has to work around the #define of BLOCK_SIZE.

In addition to your proposal and my patch, there is also the macro dbtob(), which is present at least on glibc, uclibc and musl. (ddtob(x) = x * BLOCK_SIZE = x * 1024) However I find the macro name not very descriptive.
(In reply to Felix Janda from comment #2)
> First, I've found the relevant previous bug report:
> https://bugzilla.mozilla.org/show_bug.cgi?id=918140
> Note that I would have nothing to report if the patch
> https://bugzilla.mozilla.org/attachment.cgi?id=8522932 had been committed. I
> also see that my patch should also remove the <sys/mount.h> include. Should
> I ask someone from this bug report for input?

You can if you like.  It looks like reviewers requested BLOCK_SIZE to be used (bug 918140 comment 8 and bug 918140 comment 9) instead of the bare 1024.  We can debate where BLOCK_SIZE should come from, but I agree with them that some named thing should be used instead of bare numbers.

> I think (I can also ask) that no BLOCK_SIZE in sys/mount.h is a feature.
> BLOCK_SIZE is a fairly generic name which might collide with other code
> ("name space pollution") and I don't see it documented. For example
> http://git.busybox.net/busybox/tree/util-linux/minix.
> h?id=606291beabab14c85a141c7a4225fbcab8d19fbd#n54 has to work around the
> #define of BLOCK_SIZE.

That's a good point.  I guess the question is whether any code including <sys/mount.h> is likely to use the BLOCK_SIZE identifier for its own purposes, or the BLOCK_SIZE identifier in conjunction with...other kernel disk-ish APIs (since nothing in <sys/mount.h> would really use it).

But that's really a question for the musl maintainers, and mostly orthogonal to what the issue is here.

> In addition to your proposal and my patch, there is also the macro dbtob(),
> which is present at least on glibc, uclibc and musl. (ddtob(x) = x *
> BLOCK_SIZE = x * 1024) However I find the macro name not very descriptive.

I would have thought it was "disk blocks to bytes", but the glibc <sys/quota.h> says "Converts diskblocks to blocks [and the other way around]."  Perhaps the comment is simply wrong.

I would be fine using that macro, too.  It's not the greatest name, but at least somebody familiar with the quotactl API is likely to be familiar with the macro too.  And a comment can be provided in nsLocalFileUnix.cpp similar to what's already there to explain the usage of the macro.
Assignee

Comment 4

4 years ago
After some more investigation: BLOCK_SIZE seems to denote the size of the blocks the linux kernel uses as primitive units for its buffer cache. So the macro makes sense in <linux/fs.h>.

> > In addition to your proposal and my patch, there is also the macro dbtob(),
> > which is present at least on glibc, uclibc and musl. (ddtob(x) = x *
> > BLOCK_SIZE = x * 1024) However I find the macro name not very descriptive.
> 
> I would have thought it was "disk blocks to bytes", but the glibc
> <sys/quota.h> says "Converts diskblocks to blocks [and the other way
> around]."  Perhaps the comment is simply wrong.

You are right, that seems to be a typo. The BSDs have a similar macro in sys/param.h but for 512 byte blocks.

> I would be fine using that macro, too.  It's not the greatest name, but at
> least somebody familiar with the quotactl API is likely to be familiar with
> the macro too.  And a comment can be provided in nsLocalFileUnix.cpp similar
> to what's already there to explain the usage of the macro.

I'll go with this variant.
Assignee

Comment 5

4 years ago
Posted patch Proposed patch v2 (obsolete) — Splinter Review
Attachment #8589471 - Attachment is obsolete: true
Assignee

Comment 6

4 years ago
Went now essentially with your version after all. The patch cannot really break anything which worked before.
Attachment #8589894 - Attachment is obsolete: true
Attachment #8607171 - Flags: review?(nfroyd)
Attachment #8607171 - Flags: review?(nfroyd) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd517a7fc20f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.