[pacman-dev] [PATCH] Check mountpoint read-only status when checking space
This is a bit of a stopgap solution for the problem, but an easier one than revamping the file conflict checking code to support the same stuff. Using some more gross autoconf magic, figure out which struct field we need to look at to determine read-only status and store that on our mountpoint struct. If we find out we needed this partition after calculating size requirements, then toss an error. Signed-off-by: Dan McGee <dan@archlinux.org> --- Note: there are two definite areas for improvement here- we would fail on a package uninstall due to us not setting the ->used flag there. We may want two flags for this purpose- used, and "getting installed to" or something. Second is we don't do anything with directories and symlinks, so this would still fail if /boot was read-only: /var/lib/myfile /boot/youlose /boot/youloseagain -> /var/lib/myfile -Dan configure.ac | 4 ++++ lib/libalpm/diskspace.c | 30 +++++++++++++++++++++++------- lib/libalpm/diskspace.h | 1 + 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/configure.ac b/configure.ac index 47d6093..1039bba 100644 --- a/configure.ac +++ b/configure.ac @@ -194,6 +194,10 @@ AC_CHECK_FUNCS([geteuid getmntinfo realpath regcomp strcasecmp \ wcwidth uname]) # For the diskspace code FS_STATS_TYPE +AC_CHECK_MEMBERS([struct statvfs.f_flag],,,[[#include <sys/statvfs.h>]]) +AC_CHECK_MEMBERS([struct statfs.f_flags],,,[[#include <sys/param.h> + #include <sys/mount.h>]]) + # Enable large file support if available AC_SYS_LARGEFILE diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index ae2edf7..d47f9be 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -19,6 +19,7 @@ #include "config.h" +#include <errno.h> #if defined(HAVE_MNTENT_H) #include <mntent.h> #endif @@ -66,7 +67,7 @@ static alpm_list_t *mount_point_list(void) #if defined HAVE_GETMNTENT struct mntent *mnt; FILE *fp; - FSSTATSTYPE fsp; + struct statvfs fsp; fp = setmntent(MOUNTED, "r"); @@ -75,16 +76,22 @@ static alpm_list_t *mount_point_list(void) } while((mnt = getmntent(fp))) { + if(!mnt) { + _alpm_log(PM_LOG_WARNING, _("could not get filesystem information\n")); + continue; + } if(statvfs(mnt->mnt_dir, &fsp) != 0) { _alpm_log(PM_LOG_WARNING, - _("could not get filesystem information for %s\n"), mnt->mnt_dir); + _("could not get filesystem information for %s: %s\n"), + mnt->mnt_dir, strerror(errno)); continue; } MALLOC(mp, sizeof(alpm_mountpoint_t), RET_ERR(PM_ERR_MEMORY, NULL)); mp->mount_dir = strdup(mnt->mnt_dir); - mp->mount_dir_len = strlen(mnt->mnt_dir); - memcpy(&(mp->fsp), &fsp, sizeof(FSSTATSTYPE)); + mp->mount_dir_len = strlen(mp->mount_dir); + memcpy(&(mp->fsp), &fsp, sizeof(struct statvfs)); + mp->read_only = fsp.f_flag & ST_RDONLY; mp->blocks_needed = 0l; mp->max_blocks_needed = 0l; @@ -101,14 +108,19 @@ static alpm_list_t *mount_point_list(void) entries = getmntinfo(&fsp, MNT_NOWAIT); if (entries < 0) { - return NULL; + return(NULL); } for(; entries-- > 0; fsp++) { MALLOC(mp, sizeof(alpm_mountpoint_t), RET_ERR(PM_ERR_MEMORY, NULL)); mp->mount_dir = strdup(fsp->f_mntonname); - mp->mount_dir_len = strlen(mnt->mnt_dir); + mp->mount_dir_len = strlen(mp->mount_dir); memcpy(&(mp->fsp), fsp, sizeof(FSSTATSTYPE)); +#if defined HAVE_STRUCT_STATVFS_F_FLAG + mp->read_only = fsp.f_flag & ST_RDONLY; +#elif defined HAVE_STRUCT_STATFS_F_FLAGS + mp->read_only = fsp.f_flags & MNT_RDONLY; +#endif mp->blocks_needed = 0l; mp->max_blocks_needed = 0l; @@ -303,7 +315,11 @@ int _alpm_check_diskspace(pmtrans_t *trans, pmdb_t *db_local) for(i = mount_points; i; i = alpm_list_next(i)) { alpm_mountpoint_t *data = i->data; - if(data->used == 1) { + if(data->used && data->read_only) { + _alpm_log(PM_LOG_ERROR, _("Partition %s is mounted read only\n"), + data->mount_dir); + abort = 1; + } else if(data->used) { /* cushion is roughly min(5% capacity, 20MiB) */ long fivepc = ((long)data->fsp.f_blocks / 20) + 1; long twentymb = (20 * 1024 * 1024 / (long)data->fsp.f_bsize) + 1; diff --git a/lib/libalpm/diskspace.h b/lib/libalpm/diskspace.h index ae99d0c..7c7dceb 100644 --- a/lib/libalpm/diskspace.h +++ b/lib/libalpm/diskspace.h @@ -37,6 +37,7 @@ typedef struct __alpm_mountpoint_t { long blocks_needed; long max_blocks_needed; int used; + int read_only; FSSTATSTYPE fsp; } alpm_mountpoint_t; -- 1.7.4
On 09/02/11 13:23, Dan McGee wrote:
This is a bit of a stopgap solution for the problem, but an easier one than revamping the file conflict checking code to support the same stuff. Using some more gross autoconf magic, figure out which struct field we need to look at to determine read-only status and store that on our mountpoint struct. If we find out we needed this partition after calculating size requirements, then toss an error.
Signed-off-by: Dan McGee<dan@archlinux.org> --- Note: there are two definite areas for improvement here- we would fail on a package uninstall due to us not setting the ->used flag there. We may want two flags for this purpose- used, and "getting installed to" or something. Second is we don't do anything with directories and symlinks, so this would still fail if /boot was read-only:
/var/lib/myfile /boot/youlose /boot/youloseagain -> /var/lib/myfile
-Dan
configure.ac | 4 ++++ lib/libalpm/diskspace.c | 30 +++++++++++++++++++++++------- lib/libalpm/diskspace.h | 1 + 3 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac index 47d6093..1039bba 100644 --- a/configure.ac +++ b/configure.ac @@ -194,6 +194,10 @@ AC_CHECK_FUNCS([geteuid getmntinfo realpath regcomp strcasecmp \ wcwidth uname]) # For the diskspace code FS_STATS_TYPE +AC_CHECK_MEMBERS([struct statvfs.f_flag],,,[[#include<sys/statvfs.h>]]) +AC_CHECK_MEMBERS([struct statfs.f_flags],,,[[#include<sys/param.h> + #include<sys/mount.h>]]) + # Enable large file support if available AC_SYS_LARGEFILE
diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index ae2edf7..d47f9be 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -19,6 +19,7 @@
#include "config.h"
+#include<errno.h> #if defined(HAVE_MNTENT_H) #include<mntent.h> #endif @@ -66,7 +67,7 @@ static alpm_list_t *mount_point_list(void) #if defined HAVE_GETMNTENT struct mntent *mnt; FILE *fp; - FSSTATSTYPE fsp; + struct statvfs fsp;
I am missing why this change (and the similar one later in the patch) are made. Was it for clarity? And if so, why not do it everywhere? e.g. two very similar memcpy lines: Changed FSSTATSTYPE:
- mp->mount_dir_len = strlen(mnt->mnt_dir); - memcpy(&(mp->fsp),&fsp, sizeof(FSSTATSTYPE)); + mp->mount_dir_len = strlen(mp->mount_dir); + memcpy(&(mp->fsp),&fsp, sizeof(struct statvfs)); + mp->read_only = fsp.f_flag& ST_RDONLY;
Not changed FSSTATSTYPE:
mp->mount_dir = strdup(fsp->f_mntonname); - mp->mount_dir_len = strlen(mnt->mnt_dir); + mp->mount_dir_len = strlen(mp->mount_dir); memcpy(&(mp->fsp), fsp, sizeof(FSSTATSTYPE));
Otherwise, Signed-off-by: Allan
On 09/02/11 13:23, Dan McGee wrote:
This is a bit of a stopgap solution for the problem, but an easier one than revamping the file conflict checking code to support the same stuff. Using some more gross autoconf magic, figure out which struct field we need to look at to determine read-only status and store that on our mountpoint struct. If we find out we needed this partition after calculating size requirements, then toss an error.
Signed-off-by: Dan McGee<dan@archlinux.org> --- Note: there are two definite areas for improvement here- we would fail on a package uninstall due to us not setting the ->used flag there. We may want two flags for this purpose- used, and "getting installed to" or something. Second is we don't do anything with directories and symlinks, so this would still fail if /boot was read-only:
/var/lib/myfile /boot/youlose /boot/youloseagain -> /var/lib/myfile
-Dan
configure.ac | 4 ++++ lib/libalpm/diskspace.c | 30 +++++++++++++++++++++++------- lib/libalpm/diskspace.h | 1 + 3 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac index 47d6093..1039bba 100644 --- a/configure.ac +++ b/configure.ac @@ -194,6 +194,10 @@ AC_CHECK_FUNCS([geteuid getmntinfo realpath regcomp strcasecmp \ wcwidth uname]) # For the diskspace code FS_STATS_TYPE +AC_CHECK_MEMBERS([struct statvfs.f_flag],,,[[#include<sys/statvfs.h>]]) +AC_CHECK_MEMBERS([struct statfs.f_flags],,,[[#include<sys/param.h> + #include<sys/mount.h>]]) + # Enable large file support if available AC_SYS_LARGEFILE
diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index ae2edf7..d47f9be 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -19,6 +19,7 @@
#include "config.h"
+#include<errno.h> #if defined(HAVE_MNTENT_H) #include<mntent.h> #endif @@ -66,7 +67,7 @@ static alpm_list_t *mount_point_list(void) #if defined HAVE_GETMNTENT struct mntent *mnt; FILE *fp; - FSSTATSTYPE fsp; + struct statvfs fsp;
I am missing why this change (and the similar one later in the patch) are made.
Was it for clarity? And if so, why not do it everywhere? This one was for clarity. In the GETMNTENT section, we explicitly call statvfs, so assuming fsp can be any other type is pretty silly. It also helps make clearer why we don't have to do the #ifdef shuffle for
On Wed, Feb 9, 2011 at 7:47 PM, Allan McRae <allan@archlinux.org> wrote: the read-only flags bit in the ENT section, only in the INFO section.
e.g. two very similar memcpy lines:
Changed FSSTATSTYPE:
- mp->mount_dir_len = strlen(mnt->mnt_dir); - memcpy(&(mp->fsp),&fsp, sizeof(FSSTATSTYPE)); + mp->mount_dir_len = strlen(mp->mount_dir); + memcpy(&(mp->fsp),&fsp, sizeof(struct statvfs)); + mp->read_only = fsp.f_flag& ST_RDONLY;
Not changed FSSTATSTYPE:
mp->mount_dir = strdup(fsp->f_mntonname); - mp->mount_dir_len = strlen(mnt->mnt_dir); + mp->mount_dir_len = strlen(mp->mount_dir); memcpy(&(mp->fsp), fsp, sizeof(FSSTATSTYPE));
So in a previous patch I actually slipped a compile-error into the GETMNTINFO section; I tried to refer to mnt->mnt_dir which doesn't exist at all. mnt isn't even a local variable there. That might explain some of the oddness when looking at the diff itself. If I split this patch into a cleanup one first, I think it might make things a bit clearer. -Dan
participants (3)
-
Allan McRae
-
Dan McGee
-
Dan McGee