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.

Sign up for GitHub

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

CLEANUP: Move event callback functions to mc_util files #713

Merged
merged 2 commits into from
Mar 5, 2024
Merged

CLEANUP: Move event callback functions to mc_util files #713

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46cfc14
CLEANUP: Move event callback functions to mc_util files
ing-eoking Mar 4, 2024
264c9c0
TEST: Unset an environment variable for topkeys
ing-eoking Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions mc_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,3 +622,43 @@ ENGINE_ERROR_CODE tokenize_sblocks(mblck_list_t *blist, int keylen, int keycnt,
return tokenize_mblocks(blist, keylen-2, keycnt, maxklen,
must_backward_compatible, tokens);
}

/*
* event callback functions
*/

/* event handlers structure */
static struct engine_event_handler {
EVENT_CALLBACK cb;
const void *cb_data;
struct engine_event_handler *next;
} *engine_event_handlers[MAX_ENGINE_EVENT_TYPE + 1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

개인적으로 기존처럼 구조체 정의와 배열 선언이 분리되어 있는 편이 더 읽기 좋다고 생각합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

engine_event_handlers 배열이 초기화되는 것이 보장되나요?

Copy link
Collaborator Author

@ing-eoking ing-eoking Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@namsic
구조체 내부 길이가 길지 않아 변수 선언과 구조체 내부가 함께 있으면,
내부 정보를 같이 볼 수 있어 코드에 대한 빠른 파악이 될 수 있다고 생각했습니다.
분리되어 있는 편이 읽기가 편하시다면 변경하겠습니다.

추가 의견으로 저는 별개로 분리 시 eigne_event_handler 구조체를 memcached.h와 같이
mc_util.h에 위치시켜 외부로 드러낼 필요는 없다고 생각됩니다.

@jhpark816
해당 구조체 변수는 global 변수이므로 OS에서 무조건 0으로 초기화가 보장됩니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

eigne_event_handler 구조체를 헤더 파일에 두지 맙시다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@namsic
의견 부탁드립니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ing-eoking
eigne_event_handler 구조체를 여기에 두더라도,
구조체 정의와 배열 선언을 분리하자는 의견입니다.

Copy link
Collaborator Author

@ing-eoking ing-eoking Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jhpark816
구조체 정의와 배열 선언을 분리할 경우
구조체 정의를 소스의 맨 위로 두거나
아래와 같이 구현되어야 합니다.

/* event handlers structure */
struct engine_event_handler {
    EVENT_CALLBACK cb;
    const void *cb_data;
    struct engine_event_handler *next;
};
static struct engine_event_handler *engine_event_handlers[MAX_ENGINE_EVENT_TYPE + 1];

예제의 형태는 크게 차이가 없다고 생각되어서
구조체 정의를 소스의 맨 위로 위치시키는게 읽기 편하신 형태인건지 궁금하여 요청드렸습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

위의 예와 같이 구현합시다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정되었습니다.


/*
* Register a callback.
*/
void register_callback(ENGINE_HANDLE *eh,
ENGINE_EVENT_TYPE type,
EVENT_CALLBACK cb, const void *cb_data)
{
struct engine_event_handler *h =
calloc(sizeof(struct engine_event_handler), 1);

assert(h);
h->cb = cb;
h->cb_data = cb_data;
h->next = engine_event_handlers[type];
engine_event_handlers[type] = h;
}

/*
* Perform all callbacks of a given type for the given connection.
*/
void perform_callbacks(ENGINE_EVENT_TYPE type,
const void *data, const void *c)
{
for (struct engine_event_handler *h = engine_event_handlers[type];
h; h = h->next) {
h->cb(c, type, data, h->cb_data);
}
}
8 changes: 8 additions & 0 deletions mc_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#ifndef MC_UTIL_H
#define MC_UTIL_H

#include <memcached/engine.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

engine.h가 아니라 callback.h만 include해도 충분할 것으로 생각합니다.

Copy link
Collaborator Author

@ing-eoking ing-eoking Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

해당 내용 수정되었습니다.

별개로 다른 모듈에서의 헤더 계층 구조도 이상하다고 생각됩니다.
engine.h에서는 extension.h를 include하고 있기에 따로 include할 이유가 없습니다.

#include <memcached/extension.h>
#include <memcached/engine.h>

위와 같이 모듈을 include하고 있는 모듈은 아래와 같습니다.

  • extension_logger
  • memcached
  • syslog_logger
  • item_base
  • mock_server(test용)

@jhpark816 @namsic

이는 해당 모듈이 extension과 engine API를 활용한다는 것을 알려주는 목적인가요?
개인적으로 저는 extension에 있는 token_t가 왜 있는지도 모르겠습니다.
util이나 types 모듈에 넣는게 낫지 않을까? 생각됩니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ing-eoking
구현 의도는 모릅니다.
여튼, 본 이슈 후에 아래 2가지 사항을 이슈로 등록하여 처리하시죠.

  • include 관계 정리
  • extension.h 파일 정리

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네. 정리하여 올리겠습니다.

#include <memcached/extension.h>

/* length of string representing 4 bytes integer is 10 */
Expand Down Expand Up @@ -95,4 +96,11 @@ ENGINE_ERROR_CODE tokenize_sblocks(mblck_list_t *blist, int keylen, int keycnt,
int maxklen, bool must_backward_compatible,
token_t *tokens);

/* event callback functions */
void register_callback(ENGINE_HANDLE *eh,
ENGINE_EVENT_TYPE type,
EVENT_CALLBACK cb, const void *cb_data);
void perform_callbacks(ENGINE_EVENT_TYPE type,
const void *data, const void *c);

#endif
27 changes: 0 additions & 27 deletions memcached.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,6 @@ static struct event_base *main_base;
struct thread_stats *default_thread_stats;
topkeys_t *default_topkeys = NULL;

static struct engine_event_handler *engine_event_handlers[MAX_ENGINE_EVENT_TYPE + 1];

#ifdef ENABLE_ZK_INTEGRATION
static char *arcus_zk_cfg = NULL;
#endif
Expand Down Expand Up @@ -443,31 +441,6 @@ void safe_close(int sfd)
}
}

// Register a callback.
static void register_callback(ENGINE_HANDLE *eh,
ENGINE_EVENT_TYPE type,
EVENT_CALLBACK cb, const void *cb_data)
{
struct engine_event_handler *h =
calloc(sizeof(struct engine_event_handler), 1);

assert(h);
h->cb = cb;
h->cb_data = cb_data;
h->next = engine_event_handlers[type];
engine_event_handlers[type] = h;
}

// Perform all callbacks of a given type for the given connection.
static void perform_callbacks(ENGINE_EVENT_TYPE type,
const void *data, const void *c)
{
for (struct engine_event_handler *h = engine_event_handlers[type];
h; h = h->next) {
h->cb(c, type, data, h->cb_data);
}
}

/*
* Free list management for connections.
*/
Expand Down
6 changes: 0 additions & 6 deletions memcached.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,6 @@ struct settings {
} extensions;
};

struct engine_event_handler {
EVENT_CALLBACK cb;
const void *cb_data;
struct engine_event_handler *next;
};

extern struct settings settings;
extern EXTENSION_LOGGER_DESCRIPTOR *mc_logger;

Expand Down
Loading