[ltt-dev] [UST PATCH 1/6] Make app socket directories per-user v2
Mathieu Desnoyers
compudj at krystal.dyndns.org
Thu Mar 31 17:04:51 EDT 2011
* Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>
> On Mar 31, 2011, at 9:27 PM, Mathieu Desnoyers wrote:
>
>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>> Changes since v1:
>>> * Document memory allocation
>>>
>>> Make a separate app socket directories for each user, providing
>>> some basic security and also the possibility of consistent cleanup.
>>>
>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>> ---
>>> libust/tracectl.c | 34 +++++++++++++++++++---------------
>>> libustcomm/ustcomm.c | 34 +++++++++++++++++++++++++++++-----
>>> libustcomm/ustcomm.h | 4 ++++
>>> 3 files changed, 52 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/libust/tracectl.c b/libust/tracectl.c
>>> index 33c7280..ae92b7e 100644
>>> --- a/libust/tracectl.c
>>> +++ b/libust/tracectl.c
>>> @@ -1221,37 +1221,41 @@ static void auto_probe_connect(struct marker
>>> *m)
>>>
>>> static struct ustcomm_sock * init_app_socket(int epoll_fd)
>>
>> Hi Nils,
>>
>> I think we really need to get a straight naming scheme for init vs
>> alloc
>> functions.
>>
>> init = initialization of a structure (never allocates)
>> alloc = allocation of a structure (maybe can also initialize it, but
>> not
>> necessarily. This would be specified by the function header
>> documentation)
>>
>> Otherwise reviewing correct allocation and free of a patch like this
>> one
>> is made much more difficult than it should, and the problem will only
>> grow as we add more code.
>>
>
> Sure, but this patch doesn't change the name of the function or its
> calling. I expect that would be a separate patch fixing consistency
> issues.
Yep.
>
> As far as strings are concerned though adding alloc and init is just
> confusing I think.
Well, let's say we have a choice. Normally, I never, ever return
allocated memory from a function that expects "free()" to be called on
the return value. I always go for a:
create_blah() (which returns a pointer to allocated memory, and
initializes it)
destroy_blah()
where the free() is embedded in the destroy. By ensuring that we always
have pairs of functions, it makes changes later on easier to do, and it
reminds the caller that the pair must be called.
So actually, I was not being strict enough when requesting that we add
"alloc" to functions that return something to be freed by "free". I'm
strongly in favor of never allowing this coding practice in UST, and
require that we always have pairs of create/destroy functions. The only
case where using "free" directly would be allowed is in the error
handling path of a "create" function that did a malloc(), and in the
"destroy" function.
And yes, this should come as a separate cleanup patch, but all the new
functions added should try to follow this scheme.
Otherwise, it's just so, so easy for memory leaks to slip into C code
developed by many contributors, and it's going to at least triple the
time it takes me to do code reviews (because most of my time will have
to be spent on making sure all the mallocs are freed across multiple
functions, rather than focusing on clear pairs of create/destroy, and
malloc/free within local function scopes).
If we need to split the creation in allocation/init, we could use, in
these cases
- alloc_blah()
- init_blah()
- free_blah()
I know I might sound a little strict here, but guide lines like this
will help us keeping a good level of code quality in UST, and will also
incidentally help me keep up with code review as development pace
increases.
Thoughts ?
Mathieu
>
> /Nils
>
>> Thank you,
>>
>> Mathieu
>>
>>> {
>>> - char *name;
>>> + char *dir_name, *sock_name;
>>> int result;
>>> - struct ustcomm_sock *sock;
>>> + struct ustcomm_sock *sock = NULL;
>>>
>>> - result = asprintf(&name, "%s/%d", SOCK_DIR, (int)getpid());
>>> + dir_name = ustcomm_user_sock_dir();
>>> + if (!dir_name)
>>> + return NULL;
>>> +
>>> + result = asprintf(&sock_name, "%s/%d", dir_name, (int)getpid());
>>> if (result < 0) {
>>> ERR("string overflow allocating socket name, "
>>> "UST thread bailing");
>>> - return NULL;
>>> + goto free_dir_name;
>>> }
>>>
>>> - result = ensure_dir_exists(SOCK_DIR);
>>> + result = ensure_dir_exists(dir_name);
>>> if (result == -1) {
>>> ERR("Unable to create socket directory %s, UST thread bailing",
>>> - SOCK_DIR);
>>> - goto free_name;
>>> + dir_name);
>>> + goto free_sock_name;
>>> }
>>>
>>> - sock = ustcomm_init_named_socket(name, epoll_fd);
>>> + sock = ustcomm_init_named_socket(sock_name, epoll_fd);
>>> if (!sock) {
>>> ERR("Error initializing named socket (%s). Check that directory"
>>> - "exists and that it is writable. UST thread bailing", name);
>>> - goto free_name;
>>> + "exists and that it is writable. UST thread bailing",
>>> sock_name);
>>> + goto free_sock_name;
>>> }
>>>
>>> - free(name);
>>> - return sock;
>>> +free_sock_name:
>>> + free(sock_name);
>>> +free_dir_name:
>>> + free(dir_name);
>>>
>>> -free_name:
>>> - free(name);
>>> - return NULL;
>>> + return sock;
>>> }
>>>
>>> static void __attribute__((constructor)) init()
>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>> index 43f4289..dce1e52 100644
>>> --- a/libustcomm/ustcomm.c
>>> +++ b/libustcomm/ustcomm.c
>>> @@ -533,6 +533,21 @@ close_sock:
>>> return -1;
>>> }
>>>
>>> +/* Returns the current users socket directory, must be freed */
>>> +char *ustcomm_user_sock_dir(void)
>>> +{
>>> + int result;
>>> + char *sock_dir = NULL;
>>> +
>>> + result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>>> + cuserid(NULL));
>>> + if (result < 0) {
>>> + ERR("string overflow allocating directory name");
>>> + return NULL;
>>> + }
>>> +
>>> + return sock_dir;
>>> +}
>>>
>>> /* Open a connection to a traceable app.
>>> *
>>> @@ -545,21 +560,30 @@ int ustcomm_connect_app(pid_t pid, int *app_fd)
>>> {
>>> int result;
>>> int retval = 0;
>>> - char *name;
>>> + char *dir_name, *sock_name;
>>> +
>>> + dir_name = ustcomm_user_sock_dir();
>>> + if (!dir_name)
>>> + return -ENOMEM;
>>>
>>> - result = asprintf(&name, "%s/%d", SOCK_DIR, pid);
>>> + result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>>> if (result < 0) {
>>> ERR("failed to allocate socket name");
>>> - return -1;
>>> + retval = -1;
>>> + goto free_dir_name;
>>> }
>>>
>>> - result = ustcomm_connect_path(name, app_fd);
>>> + result = ustcomm_connect_path(sock_name, app_fd);
>>> if (result < 0) {
>>> ERR("failed to connect to app");
>>> retval = -1;
>>> + goto free_sock_name;
>>> }
>>>
>>> - free(name);
>>> +free_sock_name:
>>> + free(sock_name);
>>> +free_dir_name:
>>> + free(dir_name);
>>>
>>> return retval;
>>> }
>>> diff --git a/libustcomm/ustcomm.h b/libustcomm/ustcomm.h
>>> index 0ec04fc..db38119 100644
>>> --- a/libustcomm/ustcomm.h
>>> +++ b/libustcomm/ustcomm.h
>>> @@ -25,6 +25,7 @@
>>> #include <ust/kcompat/kcompat.h>
>>>
>>> #define SOCK_DIR "/tmp/ust-app-socks"
>>> +#define USER_SOCK_DIR "/tmp/ust-socks-"
>>>
>>> struct ustcomm_sock {
>>> struct cds_list_head list;
>>> @@ -156,6 +157,9 @@ extern int ustcomm_req(int sock,
>>> char *res_data);
>>>
>>> extern int ustcomm_request_consumer(pid_t pid, const char *channel);
>>> +
>>> +/* Returns the current users socket directory, must be freed */
>>> +extern char *ustcomm_user_sock_dir(void);
>>> extern int ustcomm_connect_app(pid_t pid, int *app_fd);
>>> extern int ustcomm_connect_path(const char *path, int
>>> *connection_fd);
>>>
>>> --
>>> 1.7.1
>>>
>>>
>>> _______________________________________________
>>> ltt-dev mailing list
>>> ltt-dev at lists.casi.polymtl.ca
>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>
>>
>> --
>> Mathieu Desnoyers
>> Operating System Efficiency R&D Consultant
>> EfficiOS Inc.
>> http://www.efficios.com
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list