Re: e2fsprogs patch for review (updated)
On Wed, 2011-10-05 at 11:18 +0200, Svante Signell wrote:
> Samuel and Guillem,
>
> Thanks for your comments.
>
> On Wed, 2011-10-05 at 02:00 +0200, Guillem Jover wrote:
> > Hi!
> > The function scope variables will not be needed either, so they can be
> > ifdefed out too, or they might generate warnings.
> They can not be #ifdef'd on Q_V2_GETQUOTA since they are defined in the
> included header files. The only thing to check is quotactl defined in
> quota.h. Adding a configure check for that file would enable testing if
> quota.h exists and by that #ifdef out the call to quotatctl. Since quota
> is not supported in Hurd, I'll disable the build of libquota in
> debian/rules instead (except if there are other reasons for building it,
> and getting rid of the PATH_MAX stuff?).
I created a configure.in check for quota.h and #ifdef'd on that instead
since building without libquota.a created a lot of undefined references
when linking.
> The other coding comments are appreciated, but since the code is part of
> libquota I will not submit a patch on that part.
See above and the attached updated patch, the number of changes became
rather many. Guillem, I used your advice to let get_qf_name() return the
qf_name, instead of having it both in the argument list and as a return
value. free() does not seem to apply when using the const attribute!?
Ready to submit as a Debian bug?
--- e2fsprogs-1.42~WIP-2011-10-01/configure.in 2011-09-19 22:22:20.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//configure.in 2011-10-05 10:25:28.000000000 +0200
@@ -802,7 +802,7 @@
else
AC_CHECK_PROGS(BUILD_CC, gcc cc)
fi
-AC_CHECK_HEADERS(dirent.h errno.h execinfo.h getopt.h malloc.h mntent.h paths.h semaphore.h setjmp.h signal.h stdarg.h stdint.h stdlib.h termios.h termio.h unistd.h utime.h linux/falloc.h linux/fd.h linux/major.h net/if_dl.h netinet/in.h sys/disklabel.h sys/file.h sys/ioctl.h sys/mkdev.h sys/mman.h sys/prctl.h sys/queue.h sys/resource.h sys/select.h sys/socket.h sys/sockio.h sys/stat.h sys/syscall.h sys/sysmacros.h sys/time.h sys/types.h sys/un.h sys/wait.h)
+AC_CHECK_HEADERS(dirent.h errno.h execinfo.h getopt.h malloc.h mntent.h paths.h semaphore.h setjmp.h signal.h stdarg.h stdint.h stdlib.h termios.h termio.h unistd.h utime.h quota.h linux/falloc.h linux/fd.h linux/major.h net/if_dl.h netinet/in.h sys/disklabel.h sys/file.h sys/ioctl.h sys/mkdev.h sys/mman.h sys/prctl.h sys/queue.h sys/resource.h sys/select.h sys/socket.h sys/sockio.h sys/stat.h sys/syscall.h sys/sysmacros.h sys/time.h sys/types.h sys/un.h sys/wait.h)
AC_CHECK_HEADERS(sys/disk.h sys/mount.h,,,
[[
#if HAVE_SYS_QUEUE_H
diff -ur e2fsprogs-1.42~WIP-2011-10-01/e2fsck/quota.c e2fsprogs-1.42~WIP-2011-10-01.modified//e2fsck/quota.c
--- e2fsprogs-1.42~WIP-2011-10-01/e2fsck/quota.c 2011-09-18 20:17:26.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//e2fsck/quota.c 2011-10-05 12:31:28.000000000 +0200
@@ -24,7 +24,7 @@
ext2_ino_t ino;
struct ext2_inode inode;
errcode_t retval;
- char qf_name[255];
+ const char *qf_name;
if (ext2fs_read_inode(fs, from_ino, &inode))
return;
@@ -38,7 +38,7 @@
ext2fs_write_new_inode(fs, to_ino, &inode);
/* unlink the old inode */
- get_qf_name(qtype, QFMT_VFS_V1, qf_name);
+ qf_name = get_qf_name(qtype, QFMT_VFS_V1);
ext2fs_unlink(fs, EXT2_ROOT_INO, qf_name, from_ino, 0);
ext2fs_inode_alloc_stats(fs, from_ino, -1);
}
diff -ur e2fsprogs-1.42~WIP-2011-10-01/e2fsck/sigcatcher.c e2fsprogs-1.42~WIP-2011-10-01.modified//e2fsck/sigcatcher.c
--- e2fsprogs-1.42~WIP-2011-10-01/e2fsck/sigcatcher.c 2011-09-29 04:05:13.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//e2fsck/sigcatcher.c 2011-10-05 09:14:57.000000000 +0200
@@ -132,15 +132,33 @@
};
static struct str_table generic_code_table[] = {
+#ifdef SI_ASYNCNL
DEFINE_ENTRY(SI_ASYNCNL)
+#endif
+#ifdef SI_TKILL
DEFINE_ENTRY(SI_TKILL)
+#endif
+#ifdef SI_SIGIO
DEFINE_ENTRY(SI_SIGIO)
+#endif
+#ifdef SI_ASYNCIO
DEFINE_ENTRY(SI_ASYNCIO)
+#endif
+#ifdef SI_MESGQ
DEFINE_ENTRY(SI_MESGQ)
+#endif
+#ifdef SI_TIMER
DEFINE_ENTRY(SI_TIMER)
+#endif
+#ifdef SI_QUEUE
DEFINE_ENTRY(SI_QUEUE)
+#endif
+#ifdef SI_USER
DEFINE_ENTRY(SI_USER)
+#endif
+#ifdef SI_KERNEL
DEFINE_ENTRY(SI_KERNEL)
+#endif
END_TABLE
};
diff -ur e2fsprogs-1.42~WIP-2011-10-01/lib/config.h.in e2fsprogs-1.42~WIP-2011-10-01.modified//lib/config.h.in
--- e2fsprogs-1.42~WIP-2011-10-01/lib/config.h.in 2011-09-19 22:22:20.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//lib/config.h.in 2011-10-05 10:31:05.000000000 +0200
@@ -281,6 +281,9 @@
/* Define to 1 if you have the `putenv' function. */
#undef HAVE_PUTENV
+/* Define to 1 if you have the <quota.h> header file. */
+#undef HAVE_QUOTA_H
+
/* Define to 1 if dirent has d_reclen */
#undef HAVE_RECLEN_DIRENT
diff -ur e2fsprogs-1.42~WIP-2011-10-01/lib/quota/mkquota.c e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/mkquota.c
--- e2fsprogs-1.42~WIP-2011-10-01/lib/quota/mkquota.c 2011-09-18 20:06:52.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/mkquota.c 2011-10-05 12:26:13.000000000 +0200
@@ -48,12 +48,16 @@
int is_quota_on(ext2_filsys fs, int type)
{
+#ifdef HAVE_QUOTA_H
char tmp[1024];
qid_t id = (type == USRQUOTA) ? getuid() : getgid();
if (!quotactl(QCMD(Q_V2_GETQUOTA, type), fs->device_name, id, tmp))
return 1;
return 0;
+#else
+ return 1;
+#endif
}
/*
@@ -62,14 +66,15 @@
*/
int quota_file_exists(ext2_filsys fs, int qtype, int fmt)
{
- char qf_name[256];
+ const char *qf_name;
errcode_t ret;
ext2_ino_t ino;
if (qtype >= MAXQUOTAS || fmt > QFMT_VFS_V1)
return -EINVAL;
- get_qf_name(qtype, fmt, qf_name);
+ if ((qf_name = get_qf_name(qtype, fmt)) == NULL)
+ return 0;
ret = ext2fs_lookup(fs, EXT2_ROOT_INO, qf_name, strlen(qf_name), 0,
&ino);
diff -ur e2fsprogs-1.42~WIP-2011-10-01/lib/quota/mkquota.h e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/mkquota.h
--- e2fsprogs-1.42~WIP-2011-10-01/lib/quota/mkquota.h 2011-09-16 05:40:34.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/mkquota.h 2011-10-05 12:05:27.000000000 +0200
@@ -59,7 +59,7 @@
void set_sb_quota_inum(ext2_filsys fs, ext2_ino_t ino, int qtype);
/* in quotaio.c */
-const char *get_qf_name(int type, int fmt, char *buf);
+const char *get_qf_name(int type, int fmt);
const char *get_qf_path(const char *mntpt, int qtype, int fmt,
char *path_buf, size_t path_buf_size);
diff -ur e2fsprogs-1.42~WIP-2011-10-01/lib/quota/quotaio.c e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/quotaio.c
--- e2fsprogs-1.42~WIP-2011-10-01/lib/quota/quotaio.c 2011-09-29 00:34:41.000000000 +0200
+++ e2fsprogs-1.42~WIP-2011-10-01.modified//lib/quota/quotaio.c 2011-10-05 15:14:56.000000000 +0200
@@ -52,10 +52,15 @@
/**
* Creates a quota file name for given type and format.
*/
-const char *get_qf_name(int type, int fmt, char *buf)
+const char *get_qf_name(int type, int fmt)
{
- BUG_ON(!buf);
- snprintf(buf, PATH_MAX, "%s.%s",
+ int len;
+ char *buf=NULL;
+
+ len = strlen(basenames[fmt]) + 1 + strlen(extensions[type]) + 1;
+ if( (buf = malloc(len)) == NULL)
+ return NULL;
+ snprintf(buf, len, "%s.%s",
basenames[fmt], extensions[type]);
return buf;
@@ -64,8 +69,7 @@
const char *get_qf_path(const char *mntpt, int qtype, int fmt,
char *path_buf, size_t path_buf_size)
{
- struct stat qf_stat;
- char qf_name[PATH_MAX] = {0};
+ const char *qf_name;
BUG_ON(!mntpt);
BUG_ON(!path_buf);
@@ -73,7 +77,9 @@
strncpy(path_buf, mntpt, path_buf_size);
strncat(path_buf, "/", 1);
- strncat(path_buf, get_qf_name(qtype, fmt, qf_name),
+ if ((qf_name = get_qf_name(qtype, fmt)) == NULL)
+ return NULL;
+ strncat(path_buf, qf_name,
path_buf_size - strlen(path_buf));
return path_buf;
@@ -262,7 +268,6 @@
int fd = 0;
ext2_file_t e2_file;
const char *mnt_fsname;
- char qf_name[PATH_MAX];
int err;
struct ext2_inode inode;
unsigned long qf_inum;
Reply to: