santa-driver: add an acknowledge feature to allow timeouts #220

tburgin · 2017-11-27T18:25:31Z

lost requests will be retried
acknowledged requests will wait forever for a response - only large binaries generate an acknowledgement

…equests

russellhancox · 2018-01-24T19:09:04Z

Source/santa-driver/SantaDecisionManager.h

+  /**
+   While waiting for a response from the daemon, this is the maximum number cache checks before
+   re-sending the request.
+   */


russellhancox · 2018-01-24T19:09:16Z

Source/santa-driver/SantaDecisionManager.h

@@ -144,6 +144,12 @@ class SantaDecisionManager : public OSObject {
  */
  static const uint32_t kRequestLoopSleepMilliseconds = 1000;

+  /**
+   While waiting for a response from the daemon, this is the maximum number cache checks before
+   re-sending the request.


Indent both lines +2

russellhancox · 2018-01-24T19:17:09Z

Source/santa-driver/SantaDecisionManager.cc

-    } while (return_action == ACTION_REQUEST_BINARY && ClientConnected());
+      if (return_action == ACTION_REQUEST_BINARY) ++cache_check_count;
+    } while ((return_action == ACTION_REQUEST_BINARY || return_action == ACTION_RESPOND_ACK)
+             && ClientConnected() && cache_check_count < kRequestCacheChecks);


I think the condition here can include the cache_check_count increment too without being less clear. Maybe this:

} while (ClientConnected() && ((return_action == ACTION_REQUEST_BINARY && ++cache_check_count < kRequestCacheChecks) || (return_action == ACTION_RESPOND_ACK)));

Yeah, that's nicer. Except the || is over 100 lines :(

russellhancox · 2018-01-24T19:24:59Z

Source/santa-driver/SantaDecisionManager.cc

    case ACTION_RESPOND_ALLOW:
    case ACTION_RESPOND_DENY:
-      decision_cache->set(
-          identifier, val, ((uint64_t)ACTION_REQUEST_BINARY << 56));
+      if (decision_cache->set(identifier, val, ((uint64_t)ACTION_REQUEST_BINARY << 56))) {


Can you add a TODO above this line to make calling set twice unnecessary - having both calls will involve finding and locking the bucket (or buckets) multiple times.

russellhancox · 2018-01-24T19:36:31Z

Source/santad/SNTExecutionController.m

@@ -38,6 +38,8 @@
 #import "SNTStoredEvent.h"
 #import "SNTSyncdQueue.h"

+static size_t kLargeBinarySize = 30 * 1024 * 1024;


Add a brief comment.

russellhancox · 2018-01-24T19:37:25Z

Tests/KernelTests/main.mm

@@ -239,6 +242,18 @@ - (void)beginListening {
        } else if (strncmp("/bin/cat", vdata.path, strlen("/bin/cat")) == 0) {
          [self postToKernelAction:ACTION_RESPOND_ALLOW forVnodeID:vdata.vnode_id];
          self.timesSeenCat++;
+        } else if (strncmp("/usr/bin/cal", vdata.path, strlen("/usr/bin/cal")) == 0) {
+          static int count;


googlebot · 2018-01-25T00:01:03Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

tburgin

PTAL

tburgin · 2018-01-24T23:11:34Z

Source/santa-driver/SantaDecisionManager.cc

    case ACTION_RESPOND_ALLOW:
    case ACTION_RESPOND_DENY:
-      decision_cache->set(
-          identifier, val, ((uint64_t)ACTION_REQUEST_BINARY << 56));
+      if (decision_cache->set(identifier, val, ((uint64_t)ACTION_REQUEST_BINARY << 56))) {


tburgin · 2018-01-24T23:39:12Z

Source/santa-driver/SantaDecisionManager.cc

-    } while (return_action == ACTION_REQUEST_BINARY && ClientConnected());
+      if (return_action == ACTION_REQUEST_BINARY) ++cache_check_count;
+    } while ((return_action == ACTION_REQUEST_BINARY || return_action == ACTION_RESPOND_ACK)
+             && ClientConnected() && cache_check_count < kRequestCacheChecks);


Yeah, that's nicer. Except the || is over 100 lines :(

tburgin · 2018-01-24T23:40:15Z

Source/santa-driver/SantaDecisionManager.h

@@ -144,6 +144,12 @@ class SantaDecisionManager : public OSObject {
  */
  static const uint32_t kRequestLoopSleepMilliseconds = 1000;

+  /**
+   While waiting for a response from the daemon, this is the maximum number cache checks before
+   re-sending the request.


tburgin · 2018-01-24T23:40:19Z

Source/santa-driver/SantaDecisionManager.h

+  /**
+   While waiting for a response from the daemon, this is the maximum number cache checks before
+   re-sending the request.
+   */


tburgin · 2018-01-24T23:57:25Z

Source/santad/SNTExecutionController.m

@@ -38,6 +38,8 @@
 #import "SNTStoredEvent.h"
 #import "SNTSyncdQueue.h"

+static size_t kLargeBinarySize = 30 * 1024 * 1024;


tburgin · 2018-01-24T23:57:57Z

Tests/KernelTests/main.mm

@@ -239,6 +242,18 @@ - (void)beginListening {
        } else if (strncmp("/bin/cat", vdata.path, strlen("/bin/cat")) == 0) {
          [self postToKernelAction:ACTION_RESPOND_ALLOW forVnodeID:vdata.vnode_id];
          self.timesSeenCat++;
+        } else if (strncmp("/usr/bin/cal", vdata.path, strlen("/usr/bin/cal")) == 0) {
+          static int count;


* santa-driver: Add an acknowledge feature to allow timeouts for lost requests * project: cocoapods 1.3.1 update * review updates

tburgin added the bug label Nov 27, 2017

tburgin requested a review from russellhancox November 27, 2017 18:25

tburgin mentioned this pull request Nov 27, 2017

santa-driver: Add timestamp support for ACTION_REQUEST_BINARY #219

Closed

tburgin added 2 commits November 28, 2017 10:34

santa-driver: Add an acknowledge feature to allow timeouts for lost r…

24d0cb5

…equests

project: cocoapods 1.3.1 update

b7dfe0b

tburgin force-pushed the request-ack branch from 849052b to b7dfe0b Compare November 28, 2017 15:34

Merge branch 'master' into request-ack

db54d09

tburgin force-pushed the request-ack branch from 630c434 to db54d09 Compare November 28, 2017 17:00

Merge branch 'master' into request-ack

f601910

google deleted a comment from googlebot Jan 24, 2018

russellhancox reviewed Jan 24, 2018

View reviewed changes

review updates

bf4db58

tburgin commented Jan 25, 2018

View reviewed changes

russellhancox approved these changes Jan 25, 2018

View reviewed changes

tburgin merged commit 0e6eb45 into google:master Jan 26, 2018

dskfh pushed a commit to dskfh/santa that referenced this pull request Jul 17, 2020

santa-driver: add an acknowledge feature to allow timeouts (google#220)

aaca348

* santa-driver: Add an acknowledge feature to allow timeouts for lost requests * project: cocoapods 1.3.1 update * review updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

santa-driver: add an acknowledge feature to allow timeouts #220

santa-driver: add an acknowledge feature to allow timeouts #220

tburgin commented Nov 27, 2017

russellhancox Jan 24, 2018

tburgin Jan 24, 2018

russellhancox Jan 24, 2018

tburgin Jan 24, 2018

russellhancox Jan 24, 2018

tburgin Jan 24, 2018

russellhancox Jan 24, 2018

tburgin Jan 24, 2018

russellhancox Jan 24, 2018

tburgin Jan 24, 2018

russellhancox Jan 24, 2018

tburgin Jan 24, 2018

googlebot commented Jan 25, 2018

tburgin left a comment

tburgin Jan 24, 2018

tburgin Jan 24, 2018

tburgin Jan 24, 2018

tburgin Jan 24, 2018

tburgin Jan 24, 2018

tburgin Jan 24, 2018

santa-driver: add an acknowledge feature to allow timeouts #220

santa-driver: add an acknowledge feature to allow timeouts #220

Conversation

tburgin commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jan 25, 2018

tburgin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment