search

open-iscsi / tcmu-runner

A daemon that handles the userspace side of the LIO TCM-User backstore.
Apache License 2.0
190 stars 148 forks source link

Fix compilation on musl libc #664

Closed JuniorJPDJ closed 3 years ago

JuniorJPDJ commented 3 years ago

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

lxbsz commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

I reworded commits and added descriptions to them

JuniorJPDJ commented 3 years ago

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 commented 3 years ago

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

JuniorJPDJ commented 3 years ago

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

lxbsz commented 3 years ago

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 commented 3 years ago

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

lxbsz commented 3 years ago

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 commented 3 years ago

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 https://github.com/open-iscsi/tcmu-runner/pull/609/commits/9a4a7211eb27c5b87837cca9211b26ed7baaf8f8#r362956473 ?