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.

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

Missing error handlings for pthread_mutex_lock #510

Open
ycaibb opened this issue Dec 14, 2021 · 4 comments
Open

Missing error handlings for pthread_mutex_lock #510

ycaibb opened this issue Dec 14, 2021 · 4 comments
Assignees
Labels
Easy Good for new contributors Maybe Undecided whether in scope for the project

Comments

@ycaibb
Copy link

ycaibb commented Dec 14, 2021

Hi, I have a suggestion about error handlings for locking. Would it be better to handle the possible errors that return from pthread_mutex_lock. It seems there are some places missing handlings.

Handled

lxcfs/src/bindings.c

Lines 116 to 123 in 80613fb

static void mutex_lock(pthread_mutex_t *l)
{
int ret;
ret = pthread_mutex_lock(l);
if (ret)
log_exit("%s - returned %d\n", strerror(ret), ret);
}

lxcfs/src/lxcfs.c

Lines 40 to 47 in d7195f5

static void lock_mutex(pthread_mutex_t *l)
{
int ret;
ret = pthread_mutex_lock(l);
if (ret)
log_exit("%s - returned: %d\n", strerror(ret), ret);
}

Unhandled

lxcfs/src/proc_cpuview.c

Lines 318 to 323 in ac5989e

lxcfs_debug("New stat node (%d) for %s\n", cpu_count, cg);
}
pthread_mutex_lock(&node->lock);
/*

Possible situations that return errors.

EAGAIN The mutex could not be acquired because the maximum number
             of recursive locks for mutex has been exceeded.

      EINVAL The mutex was created with the protocol attribute having
             the value PTHREAD_PRIO_PROTECT and the calling thread's
             priority is higher than the mutex's current priority
             ceiling.

      ENOTRECOVERABLE
             The state protected by the mutex is not recoverable.

      EOWNERDEAD
             The mutex is a robust mutex and the process containing the
             previous owning thread terminated while holding the mutex
             lock. The mutex lock shall be acquired by the calling
             thread and it is up to the new owner to make the state
             consistent.

      EDEADLK
             The mutex type is PTHREAD_MUTEX_ERRORCHECK and the current
             thread already owns the mutex.

      EOWNERDEAD
             The mutex is a robust mutex and the previous owning thread
             terminated while holding the mutex lock. The mutex lock
             shall be acquired by the calling thread and it is up to
             the new owner to make the state consistent.

      EDEADLK
             A deadlock condition was detected.
@brauner
Copy link
Member

brauner commented Dec 14, 2021

So right now we choose the most aggressive option and simply exit because we assume that no error could occur that we would reasonably expect and therefore should try to gracefully handle. Instead we simply assume that any error must be fatal. What would you like to handle and why?

@ycaibb
Copy link
Author

ycaibb commented Dec 14, 2021

I just noticed that some places missing handling of pthread_mutex_lock while some places do (simply exit), which are not consistent. Why some places do not handle it or just miss it? Are there specific reasons?

For example,

lxcfs/src/proc_cpuview.c

Lines 318 to 323 in ac5989e

lxcfs_debug("New stat node (%d) for %s\n", cpu_count, cg);
}
pthread_mutex_lock(&node->lock);
/*

@brauner
Copy link
Member

brauner commented Dec 14, 2021

I just noticed that some places missing handling of pthread_mutex_lock while some places do (simply exit), which are not consistent. Why some places do not handle it or just miss it? Are there specific reasons?

For example,

lxcfs/src/proc_cpuview.c

Lines 318 to 323 in ac5989e

lxcfs_debug("New stat node (%d) for %s\n", cpu_count, cg);
}
pthread_mutex_lock(&node->lock);
/*

Yeah, I think it would be good to handle errors in proc_cpuview.c and proc_loadavg.c. There's some easy ones like in find_or_create_proc_stat_node() others will need a little more thinking like in load_free(). But happy to look at patches.

@mihalicyn mihalicyn added Easy Good for new contributors Maybe Undecided whether in scope for the project labels Mar 19, 2024
@sona78
Copy link

sona78 commented Apr 2, 2024

Could I be assigned to this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Good for new contributors Maybe Undecided whether in scope for the project
Development

No branches or pull requests

4 participants