Bug 216627 - multimedia/zoneminder and multimedia/zoneminder-h264: fail to build with clang 4.0
Summary: multimedia/zoneminder and multimedia/zoneminder-h264: fail to build with clan...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Kurt Jaeger
URL:
Keywords:
Depends on:
Blocks: 216008
  Show dependency treegraph
 
Reported: 2017-01-30 23:04 UTC by Jan Beich
Modified: 2017-04-14 17:23 UTC (History)
2 users (show)

See Also:
bsd: maintainer-feedback+
jbeich: merge-quarterly-


Attachments
zoneminder patch (3.46 KB, patch)
2017-02-03 17:11 UTC, Ivan
bsd: maintainer-approval-
Details | Diff
zoneminder-h264 patch (3.97 KB, patch)
2017-02-03 17:12 UTC, Ivan
bsd: maintainer-approval-
Details | Diff
qa logs (429.61 KB, application/gzip)
2017-02-03 17:14 UTC, Ivan
no flags Details
zoneminder.patch (3.22 KB, patch)
2017-02-06 07:17 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder-h264.patch (3.72 KB, patch)
2017-02-06 07:18 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder.patch (3.23 KB, patch)
2017-03-08 21:51 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder-h264.patch (3.73 KB, patch)
2017-03-08 21:52 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder.patch (3.23 KB, patch)
2017-03-08 22:11 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder-h264.patch (3.73 KB, patch)
2017-03-08 22:11 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder.patch (3.23 KB, patch)
2017-03-08 22:15 UTC, Ivan
bsd: maintainer-approval+
Details | Diff
zoneminder-h264.patch (3.74 KB, patch)
2017-03-08 22:16 UTC, Ivan
bsd: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Ivan 2017-02-03 17:11:45 UTC
Created attachment 179574 [details]
zoneminder patch
Comment 2 Ivan 2017-02-03 17:12:11 UTC
Created attachment 179575 [details]
zoneminder-h264 patch
Comment 3 Ivan 2017-02-03 17:14:36 UTC
Created attachment 179576 [details]
qa logs
Comment 4 Jan Beich freebsd_committer 2017-02-05 18:21:48 UTC
Comment on attachment 179574 [details]
zoneminder patch

Can you describe other changes? This is mainly for commit message.

> +-    if ( mem_ptr < 0 )
> ++    if ( mem_ptr < (void *)0 )
[...]
> +-    if ( mem_ptr > 0 )
> ++    if ( mem_ptr > (void *)0 )

mmap(2) returns MAP_FAILED on failure while shmat(2) returns (void *)-1. Both are greater than 0 on FreeBSD i386/amd64 but may not be so on other platforms.

> +-    if ( shmdt( mem_ptr ) < 0 )
> ++    if ( shmdt( mem_ptr ) < (void *)0 )

shmdt(2) returns int, not a pointer. I don't think you need to touch this line.

> +-    if ( (fd = fopen( path, "w" )) < 0 )
> ++    if ( (fd = fopen( path, "w" )) < (void *)0 )

fopen(3) returns NULL on failure, not a negative value. Maybe use "== NULL" instead.
Comment 5 Ivan 2017-02-05 18:28:33 UTC
I think you are right. I changed similar lines in zm_monitor.cpp blindly (however, I think they are not reached anyway). To my excuse I'm not a programmer myself.

I'll dig into functins descriptions and remake the patch.
Comment 6 Ivan 2017-02-06 07:17:34 UTC
Created attachment 179666 [details]
zoneminder.patch
Comment 7 Ivan 2017-02-06 07:18:11 UTC
Created attachment 179667 [details]
zoneminder-h264.patch
Comment 8 Ivan 2017-02-06 07:24:33 UTC
@Jan Beich
I fixed patches according your suggestion.

According mmap description, checking result against (void *)0 is OK, as MAP_FAILED is (void *) -1 in FreeBSD and Linux.

Also, new deps was discovered (used in rare special situations), so I added them as well.

PR on upstream. 
https://github.com/ZoneMinder/ZoneMinder/pull/1761
I hope to merge all our FreeBSD patches.
Comment 9 Kurt Jaeger freebsd_committer 2017-03-08 19:58:25 UTC
testbuilds@work
Comment 10 Jan Beich freebsd_committer 2017-03-08 21:08:57 UTC
(In reply to Ivan from comment #8)
> According mmap description, checking result against (void *)0 is OK,
> as MAP_FAILED is (void *) -1 in FreeBSD and Linux.

Did you miss (void *)-1 is always greater than (void *)0 ?

  Existing implementations of mmap() return the value -1 when
  unsuccessful. Since the casting of this value to type void * cannot
  be guaranteed by the ISO C standard to be distinct from a successful
  value, this volume of POSIX.1-2008 defines the symbol MAP_FAILED,
  which a conforming implementation does not return as the result of a
  successful call.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html
Comment 11 Ivan 2017-03-08 21:30:29 UTC
Well, as I said, I'm not a programmer myself.
How code should be changed? 
+     mem_ptr = (unsigned char *)shmat( shm_id, 0, 0 );
+-    if ( mem_ptr < 0 )
++    if ( mem_ptr < (void *)0 )
->
+     mem_ptr = (unsigned char *)shmat( shm_id, 0, 0 );
+-    if ( mem_ptr < 0 )
++    if ( mem_ptr != (void *)-1 )

and 
+ #if ZM_MEM_MAPPED
+-    if ( mem_ptr > 0 )
++    if ( mem_ptr > (void *)0 )
->
+ #if ZM_MEM_MAPPED
+-    if ( mem_ptr > 0 )
++    if ( mem_ptr != (void *)-1 )

?
Comment 12 Jan Beich freebsd_committer 2017-03-08 21:42:14 UTC
For mmap() return values convert "< 0" to "== MAP_FAILED" and "> 0" to "!= MAP_FAILED". For shmat() return values convert "< 0" to "== (void *)-1" and "> 0" to "!= (void *)-1".
Comment 13 Ivan 2017-03-08 21:51:44 UTC
Created attachment 180649 [details]
zoneminder.patch
Comment 14 Ivan 2017-03-08 21:52:15 UTC
Created attachment 180650 [details]
zoneminder-h264.patch
Comment 15 Ivan 2017-03-08 21:53:35 UTC
Done.
I looked through the code and mmap seems to be OK, so only shmat return values changed. I really hope this one can be committed.
Comment 16 Jan Beich freebsd_committer 2017-03-08 22:06:26 UTC
Comment on attachment 180649 [details]
zoneminder.patch

> + #if ZM_MEM_MAPPED
> +-    if ( mem_ptr > 0 )
> ++    if ( mem_ptr != (void *)-1 )

ZM_MEM_MAPPED uses mmap(), so it should be "!= MAP_FAILED" here to be POSIXly correct. The rest looks fine. Thank you.
Comment 17 Ivan 2017-03-08 22:11:09 UTC
Created attachment 180651 [details]
zoneminder.patch
Comment 18 Ivan 2017-03-08 22:11:33 UTC
Created attachment 180652 [details]
zoneminder-h264.patch
Comment 19 Jan Beich freebsd_committer 2017-03-08 22:11:58 UTC
Comment on attachment 180649 [details]
zoneminder.patch

> +CONFLICTS=	zoneminder-h264

Consider using CONFLICTS_INSTALL instead unless it affects build as well.

https://www.freebsd.org/doc/en/books/porters-handbook/conflicts.html
Comment 20 Ivan 2017-03-08 22:13:22 UTC
Right. 
It was // ZM_MEM_MAPPED, marking the end of block, that confused me.
Comment 21 Jan Beich freebsd_committer 2017-03-08 22:14:30 UTC
Kurt, given the changes here just fix existing bugs, I think, the commit should be MFH'd to 2017Q1 as well.
Comment 22 Ivan 2017-03-08 22:15:48 UTC
Created attachment 180653 [details]
zoneminder.patch
Comment 23 Ivan 2017-03-08 22:16:18 UTC
Created attachment 180654 [details]
zoneminder-h264.patch
Comment 24 Ivan 2017-03-08 22:16:58 UTC
CONFLICTS->CONFLICTS_INSTALL
Comment 25 commit-hook freebsd_committer 2017-03-12 19:42:18 UTC
A commit references this bug:

Author: pi
Date: Sun Mar 12 19:41:43 UTC 2017
New revision: 436017
URL: https://svnweb.freebsd.org/changeset/ports/436017

Log:
  multimedia/zoneminder{-h264}: fix build with clang 4.0

  PR:		216627
  Reported by:	jbeich
  Submitted by:	Ivan <bsd@abinet.ru> (maintainer)
  MFH:		2017Q1

Changes:
  head/multimedia/zoneminder/Makefile
  head/multimedia/zoneminder/files/patch-src_zm__monitor.cpp
  head/multimedia/zoneminder/files/patch-src_zmf.cpp
  head/multimedia/zoneminder-h264/Makefile
  head/multimedia/zoneminder-h264/files/patch-src_zm__monitor.cpp
  head/multimedia/zoneminder-h264/files/patch-src_zmf.cpp
Comment 26 Jan Beich freebsd_committer 2017-04-14 17:23:40 UTC
2017Q1 has reached EOL. No MFH is necessary anymore.