Bug 216627

Created attachment 179574 [details]
zoneminder patch

Created attachment 179575 [details]
zoneminder-h264 patch

Created attachment 179576 [details]
qa logs

Comment 4 Jan Beich 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.

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.

Created attachment 179666 [details]
zoneminder.patch

Created attachment 179667 [details]
zoneminder-h264.patch

@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 2017-03-08 19:58:25 UTC

testbuilds@work

Comment 10 Jan Beich 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

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 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".

Created attachment 180649 [details]
zoneminder.patch

Created attachment 180650 [details]
zoneminder-h264.patch

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 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.

Created attachment 180651 [details]
zoneminder.patch

Created attachment 180652 [details]
zoneminder-h264.patch

Comment 19 Jan Beich 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

Right. 
It was // ZM_MEM_MAPPED, marking the end of block, that confused me.

Comment 21 Jan Beich 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.

Created attachment 180653 [details]
zoneminder.patch

Created attachment 180654 [details]
zoneminder-h264.patch

CONFLICTS->CONFLICTS_INSTALL

Comment 25 commit-hook 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 2017-04-14 17:23:40 UTC

2017Q1 has reached EOL. No MFH is necessary anymore.