On 07/12/2012 02:57 PM, Jakub Filak wrote:
> * a config option DeleteUploaded was added to abrt.conf
> * an old behaviour of abrt-handle-upload was preserved
>
> * /var/spool/abrt-upload is not writable by default
> * for more details see rhbz#836988
>
> Signed-off-by: Jakub Filak <[email protected]>
> ---
> src/daemon/abrt-handle-upload | 32 +++++++++++++++++++++++++-------
> src/daemon/abrt.conf | 5 +++++
> src/daemon/abrtd.c | 2 +-
> src/include/libabrt.h | 3 +++
> src/lib/abrt_conf.c | 8 ++++++++
> 5 files changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/src/daemon/abrt-handle-upload b/src/daemon/abrt-handle-upload
> index a6ab3ae..ecdae34 100755
> --- a/src/daemon/abrt-handle-upload
> +++ b/src/daemon/abrt-handle-upload
> @@ -3,7 +3,15 @@
> # The task of this script is to unpack the file and move
> # problem data found in it to abrtd spool directory.
> #
> -# Usage: abrt-handle-upload ABRT_SPOOL_DIR UPLOAD_DIR FILENAME
> +# Usage: abrt-handle-upload ABRT_SPOOL_DIR UPLOAD_DIR FILENAME DELETE_ARCHIVE
This looks un-Unixy. Can it be an option? Say, -d or --delete?
Like this
delete_archive=false
if test x"$1" == x"-d"; then delete_archive=true; shift; fi
> @@ -41,17 +50,26 @@ test x"${archive%.tar.xz}" != x"$archive" &&
> unpacker="unxz"
>
> test "$unpacker" || print_clean_and_die "Unknown file type: '$archive'"
>
> -tempdir="remote.`date +%Y-%m-%d-%H:%M:%S.%N`.$$"
> +workingdir=`mktemp -d "abrt_handle_upload.XXXXXXXXXX" --tmpdir=/tmp`
> +[ "$?" -ne 0 ] && print_clean_and_die "Can't create '$workingdir' directory"
(1) Simpler:
cmd_which_result_you_check || print_clean_and_die "Can't create '$workingdir'
directory"
(you can use line continuation to split it if needed)
(2) "Can't create '$workingdir' directory" is likely to print
"Can't create '' directory". Not what you want :)
> +delete_on_exit="$workingdir"
> +
> +tempdir="$workingdir/remote.`date +%Y-%m-%d-%H:%M:%S.%N`.$$"
> +archive_working="$workingdir/$archive"
> +
> +lock_cmd="mv"
> +if [ "$delete_archive" == "no" ]; then
> + lock_cmd="cp"
> +fi
Why "lock"? Is it locking anything?
> -mv -- "$archive" "$archive.working" || print_clean_and_die "Can't lock
> '$archive'"
> +$lock_cmd -- "$archive" "$archive_working" || print_clean_and_die "Can't
> lock '$archive'"
It used to lock, because .working suffix is special (abrtd will not touch such
files).
Now you move it to other dir. Different operation.
$archive_working and $lock_cmd variables need better names.
"Can't lock '$archive'" message needs changing to reflect the truth.
> -delete_on_exit="$archive.working"
> -$unpacker -t -- "$archive.working" || print_clean_and_die "Verification
> error on '$archive'"
> +$unpacker -t -- "$archive_working" || print_clean_and_die "Verification
> error on '$archive'"
>
> echo "Unpacking '$archive'"
> mkdir "$tempdir" || print_clean_and_die "Can't create '$tempdir' directory"
> -delete_on_exit="$archive.working $tempdir"
> -$unpacker <"$archive.working" | tar xf - -C "$tempdir" ||
> print_clean_and_die "Can't unpack '$archive'"
> +$unpacker <"$archive_working" | tar xf - -C "$tempdir" ||
> print_clean_and_die "Can't unpack '$archive'"
> diff --git a/src/daemon/abrt.conf b/src/daemon/abrt.conf
> index 9a35f0c..c7ad417 100644
> --- a/src/daemon/abrt.conf
> +++ b/src/daemon/abrt.conf
> @@ -15,3 +15,8 @@ MaxCrashReportsSize = 1000
> # Changing dump location could cause problems with SELinux. See man
> abrt_selinux(8).
> #
> #DumpLocation = /var/spool/abrt
> +
> +# If you want to automatically clean the upload directory you have to tweak
> the
> +# selinux policy.
> +#
> +DeleteUploaded = no
> diff --git a/src/daemon/abrtd.c b/src/daemon/abrtd.c
> index d4efec4..d8d1cbf 100644
> --- a/src/daemon/abrtd.c
> +++ b/src/daemon/abrtd.c
> @@ -502,7 +502,7 @@ static gboolean handle_inotify_cb(GIOChannel *gio,
> GIOCondition condition, gpoin
> {
> xchdir(dir);
> execlp("abrt-handle-upload", "abrt-handle-upload",
> - g_settings_dump_location, dir, name, (char*)NULL);
> + g_settings_dump_location, dir, name,
> g_settings_delete_uploaded ? "yes" : "no", (char*)NULL);
> error_msg_and_die("Can't execute '%s'",
> "abrt-handle-upload");
> }
> if (pid > 0)
> diff --git a/src/include/libabrt.h b/src/include/libabrt.h
> index fff80ef..48d2ac3 100644
> --- a/src/include/libabrt.h
> +++ b/src/include/libabrt.h
> @@ -56,6 +56,9 @@ extern unsigned int g_settings_nMaxCrashReportsSize;
> extern char * g_settings_sWatchCrashdumpArchiveDir;
> #define g_settings_dump_location abrt_g_settings_dump_location
> extern char * g_settings_dump_location;
> +#define g_settings_delete_uploaded abrt_g_settings_delete_uploaded
> +extern bool g_settings_delete_uploaded;
> +
>
> #define load_abrt_conf abrt_load_abrt_conf
> int load_abrt_conf();
> diff --git a/src/lib/abrt_conf.c b/src/lib/abrt_conf.c
> index fc8a04f..0b53c21 100644
> --- a/src/lib/abrt_conf.c
> +++ b/src/lib/abrt_conf.c
> @@ -21,6 +21,7 @@
> char * g_settings_sWatchCrashdumpArchiveDir = NULL;
> unsigned int g_settings_nMaxCrashReportsSize = 1000;
> char * g_settings_dump_location = NULL;
> +bool g_settings_delete_uploaded = 0;
>
> void free_abrt_conf_data()
> {
> @@ -64,6 +65,13 @@ static void ParseCommon(map_string_h *settings, const char
> *conf_filename)
> else
> g_settings_dump_location = xstrdup("/var/spool/abrt");
>
> + value = g_hash_table_lookup(settings, "DeleteUploaded");
> + if (value)
> + {
> + g_settings_delete_uploaded = string_to_bool(value);
> + g_hash_table_remove(settings, "DeleteUploaded");
> + }
> +
> GHashTableIter iter;
> char *name;
> /*char *value; - already declared */
The rest looks ok to me.
--
vda
