プログラマ一年生のパッチと大物ハッカーのコード
「取締役退任。生涯一プログラマ宣言。」には大変な反響をいただいた。ここに記して御礼の気持ちとさせて頂く。トラックバック、ブックマークコメントなど可能な限り読んだ。過分のおほめ、励ましの言葉にあずかり、大変光栄に思う一方で厳しいお言葉も頂戴し皆様のご期待にそうよう一層精進していきたいと思った。
さて、プログラマ一年生である。最近では、MID (Mobile Internet Device)の開発にいそしんでいる。CPUもIntel Centrino Atomプロセッサだ。
ソフィアシステムズ Peartree用OS Asianux Mobile Midinux 2.5Jである。
http://www.sophia-systems.co.jp/ice/intel/Atom/PEARTREE/
インストーラを作っているのだが、先日ブートローダ(syslinux/extlinux)のバグに遭遇しそれを直すパッチを作成した。メーリングリストに報告したのが、7/2 18:35:14 PDT(米国西海岸夏時間)である。それに対する返事が、同日19:57:02 PDT(米国西海岸夏時間)、一時間ちょっとで、さっそく取り込んでくれた。その返事も素早いが、それだけではなく、リファクタリング(コードの改良)もしてくれた。
So it does (although the *real* problem is the lack of the stat and
device number comparison.) I just applied a (slightly different) patch
to this and pushed it out as 3.71-pre3; could you try it out?よしおかの超訳：バグだね(本当の問題はstatと装置番号の比較がなかった事だけど)。この問題に対するパッチ(ちょっと変更したコード)を適用し、それを3.71-pre3にいれておいた。試してみてくれる？
バグのあったコードは下記の部分である。細かい理解は必要ない。単にパターンマッチをして、上のif文と下のif文が対称じゃないことがわかればいい。
if ( (mtab = setmntent("/proc/mounts", "r")) ) {
while ( (mnt = getmntent(mtab)) ) {
if ( (!strcmp(mnt->mnt_type, "ext2") ||
!strcmp(mnt->mnt_type, "ext3")) &&
!stat(mnt->mnt_fsname, &dst) &&
dst.st_rdev == st.st_dev ) {
devname = mnt->mnt_fsname;
break;
}
}
}
if ( !devname ) {
/* Didn't find it in /proc/mounts, try /etc/mtab */
if ( (mtab = setmntent("/etc/mtab", "r")) ) {
while ( (mnt = getmntent(mtab)) ) {
devname = mnt->mnt_fsname;
break;
}
}
}
マウントされているファイルシステムのうち、ext2/ext3ファイルシステムを検索しているのであるが、上の部分は/proc/mountsで検索し、それがみつからなければ/etc/mtabで検索している。コードをじっくり眺めてみれば、下の部分のwhile(){...}で
if ( (!strcmp(mnt->mnt_type, "ext2") ||
!strcmp(mnt->mnt_type, "ext3")) &&
!stat(mnt->mnt_fsname, &dst) &&
dst.st_rdev == st.st_dev ) {
という比較がすっぽり抜けおちている。まあ、わかってしまえばトリビアなバグなのであるが現象としては謎であった。
なので、さっそくquick and dirty hackで下記のパッチを先のメーリングリストに投げたのである。
--- extlinux.c.orig 2007-02-10 20:47:08.000000000 +0000
+++ extlinux.c 2008-07-02 15:20:20.000000000 +0000
@@ -708,8 +708,13 @@
/* Didn't find it in /proc/mounts, try /etc/mtab */
if ( (mtab = setmntent("/etc/mtab", "r")) ) {
while ( (mnt = getmntent(mtab)) ) {
- devname = mnt->mnt_fsname;
- break;
+ if ( (!strcmp(mnt->mnt_type, "ext2") ||
+ !strcmp(mnt->mnt_type, "ext3")) &&
+ !stat(mnt->mnt_fsname, &dst) &&
+ dst.st_rdev == st.st_dev ) {
+ devname = mnt->mnt_fsname;
+ break;
+ }
}
}
}
7行追加のパッチである。まあ、素人目にはこれで一件落着である。
個人的には、ソフトウェア変更における、修正量最小化の法則、修正個所局所化の法則にのっとっているので、美しくはないかもいれないが、quick and dirty hackとしては合格点かと思った。
メンテナのちょっと変更したパッチというのが下記だ。
diff --git a/extlinux/main.c b/extlinux/main.c
index 0edce5a..819f74b 100644 (file)
--- a/extlinux/main.c
+++ b/extlinux/main.c
@@ -801,6 +801,32 @@ static int validate_device(const char *path, int devfd)
return (pst.st_dev == dst.st_rdev) ? 0 : -1;
}
+#ifndef __KLIBC__
+static const char *find_device(const char *mtab_file, dev_t dev)
+{
+ struct mntent *mnt;
+ struct stat dst;
+ FILE *mtab;
+ const char *devname = NULL;
+
+ mtab = setmntent(mtab_file, "r");
+ if (!mtab)
+ return NULL;
+
+ while ( (mnt = getmntent(mtab)) ) {
+ if ( (!strcmp(mnt->mnt_type, "ext2") ||
+ !strcmp(mnt->mnt_type, "ext3")) &&
+ !stat(mnt->mnt_fsname, &dst) && dst.st_rdev == dev ) {
+ devname = strdup(mnt->mnt_fsname);
+ break;
+ }
+ }
+ endmntent(mtab);
+
+ return devname;
+}
+#endif
+
int
install_loader(const char *path, int update_only)
{
@@ -808,11 +834,6 @@ install_loader(const char *path, int update_only)
int devfd, rv;
const char *devname = NULL;
struct statfs sfs;
-#ifndef __KLIBC__
- struct mntent *mnt = NULL;
- struct stat dst;
- FILE *mtab;
-#endif
if ( stat(path, &st) || !S_ISDIR(st.st_mode) ) {
fprintf(stderr, "%s: Not a directory: %s\n", program, path);
@@ -848,35 +869,17 @@ install_loader(const char *path, int update_only)
#else
- if ( (mtab = setmntent("/proc/mounts", "r")) ) {
- while ( (mnt = getmntent(mtab)) ) {
- if ( (!strcmp(mnt->mnt_type, "ext2") ||
- !strcmp(mnt->mnt_type, "ext3")) &&
- !stat(mnt->mnt_fsname, &dst) &&
- dst.st_rdev == st.st_dev ) {
- devname = mnt->mnt_fsname;
- break;
- }
- }
- }
-
- if ( !devname ) {
+ devname = find_device("/proc/mounts", st.st_dev);
+ if (!devname) {
/* Didn't find it in /proc/mounts, try /etc/mtab */
- if ( (mtab = setmntent("/etc/mtab", "r")) ) {
- while ( (mnt = getmntent(mtab)) ) {
- devname = mnt->mnt_fsname;
- break;
- }
- }
+ devname = find_device("/etc/mtab", st.st_dev);
}
-
- if ( !devname ) {
+ if (!devname) {
fprintf(stderr, "%s: cannot find device for path %s\n", program, path);
return 1;
}
fprintf(stderr, "%s is device %s\n", path, devname);
-
#endif
if ( (devfd = open(devname, O_RDWR|O_SYNC)) < 0 ) {
ちょっとどころの騒ぎではない。全面改訂。随分コード量が増えている。
元のコードでは、if文でext2/ext3の比較をくりかえしていたのを、ばっさりカットして、find_device()という関数にまとめた。
そのおかげで、下記のように本体はすっきりした。
devname = find_device("/proc/mounts", st.st_dev);
if (!devname) {
/* Didn't find it in /proc/mounts, try /etc/mtab */
devname = find_device("/etc/mtab", st.st_dev);
}
繰り返しをまとめるという基本中の基本である。ext2/ext3への比較などはfind_device()という新規に作った関数に閉じこめている。すっきりリファクタリングである。
syslinux一年生のコードをリファクタリングまでしてとりこんでくれたのである。
わたしは、著者のH. Peter Anvin (超大物カーネルハッカー)に会ったことも話したこともない。彼は、見ず知らずのどこの馬の骨ともわからない人間のパッチを軽やかに咀嚼して取り込んだのである。
ここまで一時間ちょっと。
オープンソースそしてバザールモデルというのはとてつもないソフトウェア開発方法論である。
まだまだ学ぶ機会に満ちている。素晴しいではないか。愉快ではないか。
メーリングリストでのやりとり。http://syslinux.zytor.com/archives/2008-July/010245.html
http://syslinux.zytor.com/archives/2008-July/010247.html
FILEって、stdio絡みでこんな処理の関数があるんですか > Linux （ってそこかw）
>修正量最小化の法則、修正個所局所化の法則
自分なんかは、他人が見てるのは主にコミットのdiffだけだろうと思うとつい「diffが分かりやすい修正にしておこうかなぁ」と思うことがあります。
が、それは今回のように、大域的にはあまりよくない場合もあり...
投稿： 通りすがりです | 2008年7月14日 (月) 22:52
通りすがりさん、
やはりパッチ投稿者としては大規模な修繕よりも細かい修繕に行きがちですが、今回学んだように大局的な視点もやっぱり重要なんですね。
実践はなかなか難しいですけど。
投稿： hyoshiok | 2008年7月15日 (火) 11:02