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

fix for issue #48 and logging fix #72

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

JMPercival
Copy link

No description provided.

log = milterContext.uuid+" Sending "+ str(milterContext.qid)+" to "+ SERVER_ENDPOINT
self.logger.writeLog(syslog.LOG_DEBUG, "%s"%(str(log)))
myhostname = socket.gethostname()
externalObject = ExternalObject(
buffer=milterContext.fileBuffer,
externalVars=ExternalVars(
filename=milterContext.archiveFileName,
source=milterContext.milterConfig.milterName+"-"+str(myhostname[:myhostname.index(".")]),
source=milterContext.milterConfig.milterName+"-"+ \
(str(myhostname[:myhostname.index(".")]) if "." in myhostname else str(myhostname)),
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using myhostname.split(".")[0] instead? seems cleaner.

Copy link
Author

Choose a reason for hiding this comment

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

Your right, I didn't realize splitting on something that might not exist would just throw it into the first entry of the list. That would be a better fix.

@ewalkup
Copy link

ewalkup commented Dec 21, 2017

@marnao A new commit has been added that fixes the non-string child object bug (it caused the worker to crash, an earlier pull was a partial fix for this). I see that this request hasn't been active - are there any other changes we need to make to JMPercival's work?

@ewalkup
Copy link

ewalkup commented Mar 15, 2018

@marnao Checking in again - these fixes have worked long-term for us, this should be ready to integrate. Let me know if it requires more changes.

@marnao
Copy link
Contributor

marnao commented Mar 26, 2018

I am rolling these into our baseline for testing. I will merge as soon as I verify.

@ewalkup
Copy link

ewalkup commented Sep 6, 2018

@marnao Just checking in again. We have other stuff we want to submit but we don't want more stuff in this pull request probably.

if not type(child_buffer) is str:
raise Exception("Buffer of %s found, not creating child scanObject" % str(type(child_buffer)))

child_buffer = ensureNotUnicode(child_buffer)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have a logic error here. This function will never get called for a unicode object because you're raising an exception above. Therefore instead of a unicode object getting encoded to a utf-8 string, you'd get an exception instead. Can you address this?

Copy link

@ewalkup ewalkup Oct 1, 2018

Choose a reason for hiding this comment

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

I think you're right - we've run initial tests and put the code in our environment, we'll push the fix once it's run for awhile/been monitored for errors.

Moving a line around so that unicode is dealt with before throwing an error.
@ewalkup
Copy link

ewalkup commented Oct 22, 2018

@marnao Pushed a fix - just moved that one line around. We have the same fix running in our environment and haven't seen it cause any errors (though we don't get unicode often).

gwalkup and others added 6 commits April 14, 2022 15:44
…es 3.support max memory threshold in redis queue 4. better handle not using ldap directory 5. better handle docker issues around mounting files vs directories 6. fix issues parsing base64 with invalid padding 7. fix lnk parser and pull out more metadata, 8. better logging, 9. support ARM process for mac binary metadata, 10. better documenation in readme, 11. change default container name ---WARNING BREAKING change
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.

5 participants