Code review comment for lp:~zhuzhaoyuan/mysql-proxy/zhuzhaoyuan

Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :

Sorry, maybe I should not believe the POSIX specification, which cheated me :(
(
http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/resource.h.html
rlim_t
    Unsigned integer type used for limit values.
)

> ...
> On Linux it seems to be a signed int64...

Not exactly, I'm afraid. It's an _unsigned long int_ on Linux. And all my Linux systems on hand proved this.

#include <sys/resource.h>
#include <stdio.h>

/**
 * see:
 * /usr/include/sys/resource.h
 * /usr/include/bits/resource.h
 * /usr/include/bits/typesizes.h
 * /usr/include/bits/types.h
 */

int main(int argc, char *argv[])
{
    printf("sizeof(rlim_t) = %d\n", sizeof(rlim_t));
    return 0;
}

$ cat /etc/issue
Ubuntu 8.04.2 \n \l
$ uname -a
Linux joshua 2.6.24-23-generic #1 SMP Mon Jan 26 00:13:11 UTC 2009 i686 GNU/Linux
$ ./a.out
sizeof(rlim_t) = 4

$ cat /etc/redhat-release
CentOS release 5.2 (Final)
$ uname -a
Linux test98.com 2.6.27.4 #1 SMP Thu Nov 6 13:33:18 CST 2008 i686 i686 i386 GNU/Linux
$ ./a.out
sizeof(rlim_t) = 4

$ cat /etc/redhat-release
CentOS release 5 (Final)
$ uname -a
Linux test64 2.6.23.12 #1 SMP Wed Jun 4 13:09:38 CST 2008 x86_64 x86_64 x86_64 GNU/Linux
$ ./a.out
sizeof(rlim_t) = 8

$ cat /etc/gentoo-release
Gentoo Base System release 1.12.11.1
$ uname -a
Linux Test-DB-Server 2.6.27-gentoo-r8 #1 SMP Mon Feb 9 16:22:38 CST 2009 x86_64 AMD Athlon(tm) 64 X2 Dual Core Processor 5200+ AuthenticAMD GNU/Linux
$ ./a.out
sizeof(rlim_t) = 8

Sadly, I have no other unix systems available. But I think treating rlim_t as unsigned long on other platforms should be fine as well (e.g. unsigned long is still 64 bits long on your Mac).
This is a trivial problem though (just debug message).

> 2) Raising the hard limit needs root permissions.

Sure.

> Now, you could argue that setting rlim_max = max_open_files will work, because either
> a) max_open_files is lower then rlim_max and if it's lower then we are fine or
> b) max_open_files is larger then rlim_max (and transitively is larger than rlim_cur) thus we will fail anyway.
> My only objection is that there's no real value in modifying the hard limit, because it would not make a difference.
> I doubt that we would get a signal for exceeding the soft limit of NOFILES, but I might be wrong. If we do, we could try to further increase the soft limit, but if we lowered the hard limit that would not be an option any more. Of course, none of that code has been written yet.

Imagine I'm the root user and rlim_max now is 1024 (actually default value on most systems). When I raise the max_open_files to any number greater than 1024, say 8192, it won't work at all. Is it too confusing, since I'm already root??
By the way, IMHO, setting both the values of rlim_cur and rlim_max is a conventional way in many server programs.
So, how about this?

=== modified file 'src/chassis.c'
--- src/chassis.c 2009-01-15 17:45:43 +0000
+++ src/chassis.c 2009-02-21 07:47:52 +0000
@@ -980,19 +980,20 @@
       G_STRLOC, g_strerror(errno), errno);
  } else {
   rlim_t soft_limit = max_files_rlimit.rlim_cur;
- g_debug("%s: current RLIMIT_NOFILE = %llu (hard: %llu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
+ g_debug("%s: current RLIMIT_NOFILE = %lu (hard: %lu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);

   max_files_rlimit.rlim_cur = max_files_number;
+ max_files_rlimit.rlim_max = MAX(max_files_number, max_files_rlimit.rlim_max);

- g_debug("%s: trying to set new RLIMIT_NOFILE = %llu (hard: %llu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
+ g_debug("%s: trying to set new RLIMIT_NOFILE = %lu (hard: %lu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
   if (-1 == setrlimit(RLIMIT_NOFILE, &max_files_rlimit)) {
- g_critical("%s: could not raise RLIMIT_NOFILE to %u, %s (%d). Current limit still %llu.", G_STRLOC, max_files_number, g_strerror(errno), errno, soft_limit);
+ g_critical("%s: could not raise RLIMIT_NOFILE to %u, %s (%d). Current limit still %lu.", G_STRLOC, max_files_number, g_strerror(errno), errno, soft_limit);
   } else {
    if (-1 == getrlimit(RLIMIT_NOFILE, &max_files_rlimit)) {
     g_warning("%s: cannot get limit of open files for this process. %s (%d)",
         G_STRLOC, g_strerror(errno), errno);
    } else {
- g_debug("%s: set new RLIMIT_NOFILE = %llu (hard: %llu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
+ g_debug("%s: set new RLIMIT_NOFILE = %lu (hard: %lu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
    }
   }
  }

In the end, I think it doesn't matter if this revised patch is objected again. Maybe we just have different angles of view. And I'm glad to discuss these issues with you :D (Otherwise, I'll create another branch to propose the new patch, happily.)
Anyway, thank you very much :)

Joshua

« Back to merge proposal