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

Fix compilation on musl libc #664

Merged
merged 5 commits into from
Jul 14, 2021
Merged

Fix compilation on musl libc #664

merged 5 commits into from
Jul 14, 2021

Conversation

JuniorJPDJ
Copy link
Contributor

I briefly tested it on Alpine Linux and it seems to work.

tcmur_work.c Show resolved Hide resolved
@lxbsz
Copy link
Collaborator

lxbsz commented Jul 7, 2021

And could you split the commit into small ones with each one fixes a single issue.
At the same time please add what warning or error you are fixing in each commit.
Thanks.

@JuniorJPDJ
Copy link
Contributor Author

And could you split the commit into small ones with each one fixes a single issue.
At the same time please add what warning or error you are fixing in each commit.
Thanks.

Ok ;) Gimme few minutes.

@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented Jul 7, 2021

remove assert_perror (musl incompatible) is for:

/home/juniorjpdj/aports/testing/tcmu-runner/tcmu-runner/api.c:77:3: error: implicit declaration of function 'assert_perror'; did you mean 'g_assert_error'? [-Werror=implicit-function-declaration]
   77 |   assert_perror(EINVAL);
      |   ^~~~~~~~~~~~~

add missing pthread.h include is for:

In file included from /home/juniorjpdj/aports/testing/tcmu-runner/tcmu-runner/configfs.c:19:
/home/juniorjpdj/aports/testing/tcmu-runner/tcmu-runner/libtcmu_common.h:194:25: error: unknown type name 'pthread_t'
  194 | void tcmu_thread_cancel(pthread_t thread);
      |                         ^~~~~~~~~

remove pthread_getname_np calls - musl incompatible is for:

/home/juniorjpdj/aports/testing/tcmu-runner/tcmu-runner/libtcmu.c:849:6: error: implicit declaration of function 'pthread_getname_np'; did you mean 'pthread_setname_np'? [-Werror=implicit-function-declaration]
  849 |  if (pthread_getname_np(pthread_self(), cname, TCMU_THREAD_NAME_LEN))
      |      ^~~~~~~~~~~~~~~~~~

and similar - there was more than one error like this, but point is musl doesn't support this (pthread_getname_np) function

implicitly cast thread id to %lu is for:

/home/juniorjpdj/aports/testing/tcmu-runner/tcmu-runner/libtcmu.c:862:21: error: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'pthread_t' {aka 'struct __pthread *'} [-Werror=format=]                                                                                                                                                                                                              
  862 |   tcmu_dev_err(dev, "Failed to set name for thread %lu\n",
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  863 |         pthread_self());
      |         ~~~~~~~~~~~~~~
      |         |
      |         pthread_t {aka struct __pthread *}

fix time_t size in printf to be architecture agnostic is for:

src/tcmu-runner-1.5.4/main.c:660:21: error: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'time_t' {aka 'long long int'} [-Werror=format=]
  660 |   tcmu_dev_dbg(dev, "Current time %lu secs.\n", time->tv_sec);
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~
      |                                                     |
      |                                                     time_t {aka long long int}

and similar (4 errors like this) - this is error while compiling on 32-bit architectures (armhf, armv7, x86).

@JuniorJPDJ
Copy link
Contributor Author

JuniorJPDJ commented Jul 7, 2021

With this patches and one from OpenSuse guy tcmu-runner compiles on musl in Alpine Linux with 64-bit and 32-bit architectures. To this moment I tested it only on x86_64 and it seem to work but I'll check it on armv7 later today.

@JuniorJPDJ
Copy link
Contributor Author

I reworded commits and added descriptions to them

@JuniorJPDJ JuniorJPDJ requested a review from lxbsz July 7, 2021 17:06
main.c Show resolved Hide resolved
This allows to preserve compatibility with 32-bit and 64-bit architectures.
Fixes compilation problems like:

	format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'time_t' {aka 'long long int'}
This permits to preserve musl libc compatibility and fixes compile time problems like:

	error: implicit declaration of function 'assert_perror';
This fixes musl libc compatibility and fixes compilation time error like:

	error: unknown type name 'pthread_t'
This fixes compilation time errors like:

	error: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'pthread_t' {aka 'struct __pthread *'}
This allows to preserve compatiblity with musl libc which lacks this function
@JuniorJPDJ
Copy link
Contributor Author

Just tested it on armv7 (32-bit) with fbo backend.
It works very good ;)
image

This means #609 and those commits fix 32-bit issues not only for compilation but also for runtime.
..at least this part I tested

@lxbsz
Copy link
Collaborator

lxbsz commented Jul 12, 2021

Just tested it on armv7 (32-bit) with fbo backend.
It works very good ;)
...

This means #609 and those commits fix 32-bit issues not only for compilation but also for runtime.
..at least this part I tested

Cool, I will test it later this week. Thanks @JuniorJPDJ

@lxbsz lxbsz merged commit 91eaa4f into open-iscsi:master Jul 14, 2021
@JuniorJPDJ
Copy link
Contributor Author

Now it would be cool to merge #609 and it would make some packagers not patch tcmu-runner at all ;D

@lxbsz
Copy link
Collaborator

lxbsz commented Jul 15, 2021

Now it would be cool to merge #609 and it would make some packagers not patch tcmu-runner at all ;D

At the same time could help test that ? Currently I didn't have the env, for me I need to install one someday later.

@JuniorJPDJ
Copy link
Contributor Author

I already tested it, it's included in Alpine Linux patches and I was testing it with this pull.

@JuniorJPDJ JuniorJPDJ deleted the musl branch July 15, 2021 18:01
@lxbsz
Copy link
Collaborator

lxbsz commented Jul 16, 2021

I already tested it, it's included in Alpine Linux patches and I was testing it with this pull.

Sounds cool. I will work on it recently after my handy fs work.

@lxbsz
Copy link
Collaborator

lxbsz commented Jul 26, 2021

I already tested it, it's included in Alpine Linux patches and I was testing it with this pull.

Have ever hit the issue @mikechristie mentioned in 9a4a721#r362956473 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants