Opened 9 years ago

Closed 9 years ago

#366 closed enhancement (complete)

The socket creator process

Reported by: vorner Owned by: vorner
Priority: medium Milestone: Sprint-20110419
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 2.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I started implementing the privileged socket creator and I have the program itself. Now I need to implement the part talking to it in boss and find a way to pass the sockets to the components, but I'd like someone to have a look at the program while I continue on the rest.

While implementing this, I put some code to isc::util library (lib/util). Some of it needs tests, it was either tested implicitly in first versions of the program (and is still, but it would use a separate testcase) or it wasn't mine code. I'll provide the tests soon.

The code is low-level and without any objects and stuff, it seemed easier without them.

Can this be taken as some kind of incremental review (so I hear the suggestions sooner and there isn't so much to review at one go)? The changes and code are in branches/vorner-sockcreator, r3117-r3176.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by vorner

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:2 Changed 9 years ago by vorner

It can be reviewed up to r3189, I added the tests I promissed.

comment:3 Changed 9 years ago by shane

  • Milestone set to A-Team-Task-Backlog

comment:4 Changed 9 years ago by stephen

  • Milestone A-Team-Task-Backlog deleted

Milestone A-Team-Task-Backlog deleted

comment:5 Changed 9 years ago by shane

  • Milestone set to Sprint-20110405
  • Owner changed from UnAssigned to vorner

We decided we're going to merge this.

comment:6 Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned

Hello

I went trough the branch, merged master into it and resolved some conflicts. However, there were some unfinished changes (some tests for boss part of the protocol, which fail, etc).

So, there are some changes which I'd like to merge in trac366. They contain the process itself only (with some refactoring, creation of utils library, etc), but not the unfinished changes. Because I don't remember at which commit the review finished and there are some changes after the merge, would someone have a fast look into it? The changes of the branch can be seen for example by „git diff ece21d1..“.

I propose this as changelog:

[func]    vorner
A new process, b10-sockcreator, is added, which will create sockets for the rest of the system.
It is the only part which will need to keep the root privileges. However, only the process exists,
nothing can talk to it yet.

Thanks

comment:7 follow-up: Changed 9 years ago by jelte

  • Owner changed from UnAssigned to vorner

Looks ok (I assume that the moved fd_ stuff has not changed, if it has, I can take a closer look at that).

Regarding the socket creator code, I'd like to see a bit of whitespace between the macros (in fact, I'd even place them outside of the function, directly above it, to better show what the function does, but the current location is defendable as well).

It would seem the README could use a little bit more info on exactly what is sent back on success (and when).

Is the (empty) send_fd in sockcreator.cc supposed to be used anywhere? (from what I can tell only the 'real' one is used, and a dummy test for testing). I suspect this simply got left after the move.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Hello

Replying to jelte:

Regarding the socket creator code, I'd like to see a bit of whitespace between the macros (in fact, I'd even place them outside of the function, directly above it, to better show what the function does, but the current location is defendable as well).

ACK

It would seem the README could use a little bit more info on exactly what is sent back on success (and when).

I'm not sure I understand what you mean. I added something about how the socket is sent. But what do you men by when? It's just sent right away, why would it wait for anything?

Is the (empty) send_fd in sockcreator.cc supposed to be used anywhere? (from what I can tell only the 'real' one is used, and a dummy test for testing). I suspect this simply got left after the move.

Yes, it was a leftover.

comment:9 in reply to: ↑ 8 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

It would seem the README could use a little bit more info on exactly what is sent back on success (and when).

I'm not sure I understand what you mean. I added something about how the socket is sent. But what do you men by when? It's just sent right away, why would it wait for anything?

n/m; Somehow I initially read that part as 'then the socket is sent with send_fd() *over another channel*. Maybe I'm the only one, if not, making the sentence 'The answer to this is either 'S', followed by' might be a tad bit better, but your call, the current text is fine. I think the empty send_fd() function that was there might have thrown me off.

comment:10 Changed 9 years ago by vorner

  • Estimated Difficulty changed from 0.0 to 2
  • Resolution set to complete
  • Status changed from reviewing to closed

Ok, thanks, closing (the estimate is only for the work done at this time, not the original)

Note: See TracTickets for help on using tickets.