Skip to content

Change the function signature of safe_[get|set]sockopt#8331

Merged
maskit merged 2 commits intoapache:masterfrom
maskit:sockopt
Jan 26, 2022
Merged

Change the function signature of safe_[get|set]sockopt#8331
maskit merged 2 commits intoapache:masterfrom
maskit:sockopt

Conversation

@maskit
Copy link
Member

@maskit maskit commented Sep 17, 2021

The type of optval argument for [get|set]sockopt is void*, but not char*.
Some values don't fit into a char, and casting the pointer to char* causes compile error.

       int getsockopt(int sockfd, int level, int optname,
                      void *optval, socklen_t *optlen);
       int setsockopt(int sockfd, int level, int optname,
                      const void *optval, socklen_t optlen);

The type of val argument for [get|set]sockopt is void*, but not char*.
Some values don't fit into a char, and casting the pointer to char* causes compile error.
@maskit maskit added the Cleanup label Sep 17, 2021
@maskit maskit added this to the 10.0.0 milestone Sep 17, 2021
@maskit maskit self-assigned this Sep 17, 2021
@randall randall changed the title Change the function singnature of safe_[get|set]sockopt Change the function signature of safe_[get|set]sockopt Sep 17, 2021
bneradt
bneradt previously approved these changes Sep 17, 2021
int safe_setsockopt(int s, int level, int optname, char *optval, int optlevel);
int safe_getsockopt(int s, int level, int optname, char *optval, int *optlevel);
int safe_setsockopt(int s, int level, int optname, void *optval, int optlevel);
int safe_getsockopt(int s, int level, int optname, void *optval, int *optlevel);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are going to mirror the setsockopt API then shouldn't it be const void * and socklen_t for the length?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept my change minimal.

If I change int * to socklen_t * I have to deal with the error below at all places that call safe_getsockopt. I could just reinterpret_cast at all the places but I guess some of the places can declare variables as unsigned int and don't need any casts. I don't want to check and fix that.

/Users/mkitajo/src/github.com/trafficserver/iocore/eventsystem/P_UnixSocketManager.h:482:11: error: no matching function for call to 'safe_getsockopt'
  r     = safe_getsockopt(s, SOL_SOCKET, SO_SNDBUF, (char *)&bsz, &bszsz);
          ^~~~~~~~~~~~~~~
../../include/tscore/ink_sock.h:38:5: note: candidate function not viable: no known conversion from 'int *' to 'socklen_t *' (aka 'unsigned int *') for 5th argument
int safe_getsockopt(int s, int level, int optname, void *optval, socklen_t *optlevel);
    ^

I added const for optval for now.

Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

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

Sounds good to me. This is an improvement.

@maskit
Copy link
Member Author

maskit commented Oct 15, 2021

@bryancall Can you take another look? Changing char * to void * solves an issue, but I don't see any real issue on the int * type.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.

@github-actions github-actions bot added the Stale label Jan 20, 2022
@maskit
Copy link
Member Author

maskit commented Jan 20, 2022

@bryancall I left this open for you because your comment was a change request, but I'm going to merge this with Brian's approval next week if there's no response.

@github-actions github-actions bot removed the Stale label Jan 21, 2022
@maskit maskit dismissed bryancall’s stale review January 26, 2022 01:11

Suggested change is minor and there's no response.

@maskit maskit merged commit e0d0504 into apache:master Jan 26, 2022
zwoop pushed a commit that referenced this pull request Feb 2, 2022
* Change the function signature of safe_[get|set]sockopt

The type of val argument for [get|set]sockopt is void*, but not char*.
Some values don't fit into a char, and casting the pointer to char* causes compile error.

* Add const

(cherry picked from commit e0d0504)
@zwoop
Copy link
Contributor

zwoop commented Feb 2, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Feb 2, 2022
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Mar 17, 2022
* asf/9.2.x:
  Updated ChangeLog
  Prevent calling SSL_set_session in the middle of handshake (apache#8600)
  Change the function signature of safe_[get|set]sockopt (apache#8331)
  Revert fixes for apache#8539 (apache#8637)
  Update descriptions of sni.yaml.default (apache#8568)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants