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

Jump to bottom

add additional-gids for lxc-attach #4243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add additional-gids for lxc-attach #4243

wants to merge 1 commit into from

Conversation

DawningRain
Copy link

add additional-gids for lxc-attach

#4235

Signed-off-by: zhangxiaoyu zhangxiaoyu58@huawei.com

@lxc-jenkins
Copy link

This pull request didn't trigger Jenkins as its author isn't in the allow list.

An organization member must perform one of the following:

  • To have this branch tested by Jenkins, use the "ok to test" command.
  • To have a one time test done, use the "test this please" command.

Those commands are simple Github comments of the format: "jenkins: COMMAND"

@mihalicyn
Copy link
Member

jenkins: ok to test

@stgraber
Copy link
Member

jenkins: test this please

@stgraber
Copy link
Member 

../src/lxc/tools/lxc_attach.c: In function ‘my_parser’:
../src/lxc/tools/lxc_attach.c:217:21: error: implicit declaration of function ‘get_additional_gids’ [-Werror=implicit-function-declaration]
  217 |                 if (get_additional_gids(arg, &additional_gids) != 0) {
      |                     ^~~~~~~~~~~~~~~~~~~
../src/lxc/tools/lxc_attach.c:217:21: warning: nested extern declaration of ‘get_additional_gids’ [-Wnested-externs]
../src/lxc/tools/lxc_attach.c: At top level:
../src/lxc/tools/lxc_attach.c:272:12: error: static declaration of ‘get_additional_gids’ follows non-static declaration
  272 | static int get_additional_gids(const char *add_gids, lxc_groups_t *gids)
      |            ^~~~~~~~~~~~~~~~~~~
../src/lxc/tools/lxc_attach.c:217:21: note: previous implicit declaration of ‘get_additional_gids’ with type ‘int()’
  217 |                 if (get_additional_gids(arg, &additional_gids) != 0) {
      |                     ^~~~~~~~~~~~~~~~~~~
../src/lxc/tools/lxc_attach.c:272:12: warning: ‘get_additional_gids’ defined but not used [-Wunused-function]
  272 | static int get_additional_gids(const char *add_gids, lxc_groups_t *gids)
      |            ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

@stgraber stgraber added the Incomplete Waiting on more information from reporter label Dec 11, 2023
@DawningRain
add additional-gids for lxc-attach
58d7460 
Signed-off-by: zhangxiaoyu <zhangxiaoyu58@huawei.com>
@DawningRain
Copy link
Author 

../src/lxc/tools/lxc_attach.c: In function ‘my_parser’:
../src/lxc/tools/lxc_attach.c:217:21: error: implicit declaration of function ‘get_additional_gids’ [-Werror=implicit-function-declaration]
  217 |                 if (get_additional_gids(arg, &additional_gids) != 0) {
      |                     ^~~~~~~~~~~~~~~~~~~
../src/lxc/tools/lxc_attach.c:217:21: warning: nested extern declaration of ‘get_additional_gids’ [-Wnested-externs]
../src/lxc/tools/lxc_attach.c: At top level:
../src/lxc/tools/lxc_attach.c:272:12: error: static declaration of ‘get_additional_gids’ follows non-static declaration
  272 | static int get_additional_gids(const char *add_gids, lxc_groups_t *gids)
      |            ^~~~~~~~~~~~~~~~~~~
../src/lxc/tools/lxc_attach.c:217:21: note: previous implicit declaration of ‘get_additional_gids’ with type ‘int()’
  217 |                 if (get_additional_gids(arg, &additional_gids) != 0) {
      |                     ^~~~~~~~~~~~~~~~~~~
../src/lxc/tools/lxc_attach.c:272:12: warning: ‘get_additional_gids’ defined but not used [-Wunused-function]
  272 | static int get_additional_gids(const char *add_gids, lxc_groups_t *gids)
      |            ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

fixed

@stgraber
Copy link
Member

jenkins: test this please

@stgraber
Copy link
Member

@brauner @mihalicyn can one of you please review?

@stgraber stgraber removed the Incomplete Waiting on more information from reporter label Dec 11, 2023
@lxc-jenkins
Copy link

Testsuite passed

@Ilmarinen100
Copy link

it would be awesome if this was integrated

mihalicyn
mihalicyn approved these changes Mar 1, 2024
View reviewed changes
Copy link
Member

@mihalicyn mihalicyn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I have added a few small suggestions, but none of them are critical. Code looks fully correct and safe to me.

src/lxc/tools/lxc_attach.c
@@ -128,6 +131,9 @@ Options :\n\
Load configuration file FILE\n\
-u, --uid=UID Execute COMMAND with UID inside the container\n\
-g, --gid=GID Execute COMMAND with GID inside the container\n\
--additional-gids\n\
Copy link
Member

Choose a reason for hiding this comment

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

--additional-gids=GIDS_LIST

src/lxc/tools/lxc_attach.c
unsigned int readvalue;
size_t i, len;
const size_t max_gids = 32;
gid_t *g = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

__do_free gid_t *g = NULL;

src/lxc/tools/lxc_attach.c
g[i] = readvalue;
}

gids->list = g;
Copy link
Member

Choose a reason for hiding this comment

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

gids->list = move_ptr(g);

src/lxc/tools/lxc_attach.c
for (i = 0; i < len; i++) {
if (lxc_safe_uint(gids_str[i], &readvalue) != 0) {
ERROR("Invalid gid value %s", gids_str[i]);
free(g);
Copy link
Member

Choose a reason for hiding this comment

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

free(g); won't be needed if you add __do_free

src/lxc/tools/lxc_attach.c
@@ -258,6 +270,51 @@ static bool stdfd_is_pty(void)
return false;
}

static int get_additional_gids(const char *add_gids, lxc_groups_t *gids)
{
unsigned int readvalue;
Copy link
Member

Choose a reason for hiding this comment

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

you can place this variable inside the loop

@stgraber stgraber added the Incomplete Waiting on more information from reporter label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihalicyn mihalicyn mihalicyn approved these changes

@brauner brauner Awaiting requested review from brauner

Assignees
No one assigned
Labels
Incomplete Waiting on more information from reporter
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DawningRain @lxc-jenkins @mihalicyn @stgraber @Ilmarinen100