Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Upstream FreeBSD specific patches] #1761

Merged
merged 7 commits into from May 10, 2017
Merged

[Upstream FreeBSD specific patches] #1761

merged 7 commits into from May 10, 2017

Conversation

abishai
Copy link
Contributor

@abishai abishai commented Jan 31, 2017

@abishai abishai changed the title [Upstream FreeBSD specific patches] implement platform-agnostic comparison without abs() [Upstream FreeBSD specific patches] Jan 31, 2017
@abishai
Copy link
Contributor Author

abishai commented Jan 31, 2017

Here are some patches I've made during porting ZM to FreeBSD. I included C and CMAKE patches to get feedback (I'm not programmer myself and have little experience in PR system). If they are OK, I'll provide even more :)
Plan is to get rid of patches entirely.

@knight-of-ni
Copy link
Member

Thank you for submitting these patches. Some of these changes will need to be tweaked a bit.

We are making a final push to get the next release out the door. I'll provide further feedback following release.

#else
if ( (unsigned)std::abs((*psrc)-RGB_VAL(ref_colour,c)) >= RGB_VAL(threshold,c) )
#endif
unsigned int diff = ((*psrc)-RGB_VAL(ref_colour,c)) > 0 ? (*psrc)-RGB_VAL(ref_colour,c) : RGB_VAL(ref_colour,c) - (*psrc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought at first that this couldn't be correct... but it is. You know what I would like to see though? the result of RGB_VAL(ref_colour,c) being put into a variable and then used. I know the compiler likely optimises it away anyways... but I think it would be more readable...

@@ -515,8 +515,9 @@ void Logger::logPrint( bool hex, const char * const filepath, const int line, co
va_list argPtr;
struct timeval timeVal;

const char * const file = basename(filepath);

char *filecopy = strdup(filepath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we copying filepath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GNU and POSIX functions have different prototypes, one is accepting char*, another - const char*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay... but you don't need to copy it.. just use a cast

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as I'm not programmer myself, I read manuals. It was said that such casting is a bad habit as const is a pointer to read only memory location and char* to read write. Recommendation was to use another pointer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so apparently some implementations of basename will modify the input. This is for multi-threaded support apparently. So... I guess this change is actually correct. But I don't want to be doing this on every logPrint. I will see if I can refactor this to be more efficient.

@knight-of-ni
Copy link
Member

knight-of-ni commented Feb 4, 2017

@abishai I sent a PR to your master branch to revise commit 0f23809
It is a better method for determining when to check for polkit.

Note that generating pr's from your own master branch can cause problems later on. It is best to always create a new branch first and do you work from that... e.g. git checkout -b my_shiny_new_branch

@abishai
Copy link
Contributor Author

abishai commented Feb 4, 2017

@knnniggett Thank you! I'll test if it still builds.

OK, I'll try it. PR system is rather complicated to me. For example, my llvm patch was glued to this one. I tried to made it separated.

@knight-of-ni
Copy link
Member

OK, I'll try it. PR system is rather complicated to me. For example, my llvm patch was glued to this one. I tried to made it separated.

Yes, that is because you are working out of your master branch.

check for polkit only if systemd is present
@abishai
Copy link
Contributor Author

abishai commented Feb 5, 2017

I'll remake clang patch. I shouldn't blindly change similar lines as only mmap returns -1.

src/zmf.cpp Outdated
@@ -331,7 +331,7 @@ int main( int argc, char *argv[] )
Debug( 1, "Got image, writing to %s", path );

FILE *fd = 0;
if ( (fd = fopen( path, "w" )) < 0 )
if ( (fd = fopen( path, "w" )) < (void *)0 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this is incorrect... I will have to do some research to figure out what it should be. I always thought that file descriptors were integers...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I wrote that I'll remake the patch. Should be == NULL.

@abishai
Copy link
Contributor Author

abishai commented Feb 6, 2017

Revised clang patch.

@knight-of-ni knight-of-ni added this to the 1.31.0 milestone Apr 27, 2017
@@ -386,13 +394,13 @@ find_library(MYSQLCLIENT_LIBRARIES mysqlclient PATH_SUFFIXES mysql)
if(MYSQLCLIENT_LIBRARIES)
set(HAVE_LIBMYSQLCLIENT 1)
list(APPEND ZM_BIN_LIBS "${MYSQLCLIENT_LIBRARIES}")
find_path(MYSQLCLIENT_INCLUDE_DIR mysql/mysql.h)
find_path(MYSQLCLIENT_INCLUDE_DIR mysql.h /usr/local/include/mysql /usr/include/mysql)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abishai Can you try this instead?
find_path(MYSQLCLIENT_INCLUDE_DIR mysql.h PATH_SUFFIXES mysql)

@knight-of-ni knight-of-ni merged commit 74dd8ab into ZoneMinder:master May 10, 2017
@knight-of-ni
Copy link
Member

@abishai I changed find_path to use PATH_SUFFIXES before merging this. I believe it should work.
Thank you for submitting these. Now that 1.30.4 is released, we will merge any future pr's like this one much more quickly. Remember though, first create a new branch in your fork and work out of that rather than master. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(TRIVIAL) 1.30 is not compiling under FreeBSD11-ALPHA3
3 participants