wesbarnett / snap-pac

Pacman hooks that use snapper to create pre/post btrfs snapshots like openSUSE's YaST
GNU General Public License v2.0
180 stars 14 forks source link

Read configuration values from `/etc/snap-pac` #16

Closed jonasc closed 6 years ago

jonasc commented 6 years ago

This is a quick proposal on how to solve #15. I did not (yet) test it.

The idea is the following: Test whether /etc/snap-pac is a readable file and then source it. Set DESC_LIMIT only to the default value of 48 if it was not specified in the sourced file.

NicoHood commented 6 years ago

I would avoid sourcing any config file and read the variables safely via printf or any other mechanism.

wesbarnett commented 6 years ago

@NicoHood Can you give a link or source on that? I've tried searching the web but haven't found anything of value. The current version of snap-pac sources the snapper configuration files, which I've realized is a bad idea (except to get what configurations are available), so I'm in the process of having snap-pac have its own configuration files.

NicoHood commented 6 years ago

@wesbarnett Here is how you can securely read a config file into a given variable:

https://github.com/NicoHood/guestwlan/blob/master/wlanqrkeygen.sh#L69-L77

# Read guestwlan.cfg
while read -r line
do
    if echo "${line}" | grep -F = &>/dev/null
    then
        varname=$(echo "${line}" | cut -d '=' -f 1)
        config[${varname}]=$(echo "${line}" | cut -d '=' -f 2-)
    fi
done < "${config[CONFIG]}"
NicoHood commented 6 years ago

@wesbarnett Did you fix the config reading in the meantime?

In the ArchLinux Package I want to backup etc/snap-pac.conf. However it does not exist in the default installation. It would be better to create a default config with empty or commented settings. So the user easily knows what he can enable and which options could be useful for him.

Please check if the package is okay like that: https://git.archlinux.org/svntogit/community.git/commit/trunk?h=packages/snap-pac&id=43fb3c67d1f7cdb9e43659437d75c5d4e7417879

wesbarnett commented 6 years ago

I ended up going with this for the configuration file: d4813a7fdc276d5c58a02e988ee0e205264558b0

Good idea with the example/default configuration files. I'm adding two example configuration files, so you should have to add anything when packaging. I'll release 2.0.1 shortly.

NicoHood commented 6 years ago

Please add those to the makefile then.

wesbarnett commented 6 years ago

@NicoHood done

NicoHood commented 6 years ago

Could you please tag another release?

wesbarnett commented 6 years ago

@NicoHood 2.0.1 includes the Makefile and example configurations

NicoHood commented 6 years ago

I suggest renaming snap-pac.conf.example to snap-pac.conf and either disable all options or set them to the defaults. This way you can track changes/new options better when packaging (saves as .pacnew) and you also do not need to cp this config.

General question: the link modules option is a feature that solves the problem that (most of the time) usb modules are not available anymore, so you cannot plug a new usb stick until the next reboot? I am thinking of enabling it then, sounds like a nice option. Did I understand that correct?

Have you also considered the case when you update a kernel, do not reboot and update the kernel again (as you've waited to long and another update arrived)?

The manpage does not state how to add multiple kernel packages, how can I do that? I use linux and linux-lts (like most users I guess).

wesbarnett commented 6 years ago

The problem with setting snap-pac.conf with all the defaults on is that you can't. The default pre and post descriptions are gathered from information that is only available when pacman is run (which packages are installed, etc). Why not use pacnew for the example files?

You're correct in your understand of the link modules feature

Have you also considered the case when you update a kernel, do not reboot and update the kernel again (as you've waited to long and another update arrived)?

I have not, so it might fail in that case. I think it will just try at creating another symlink but fail, since it will already exist.

The manpage does not state how to add multiple kernel packages, how can I do that? I use linux and linux-lts (like most users I guess).

You can't currently. Probably should add support for that.

NicoHood commented 6 years ago

The problem with setting snap-pac.conf with all the defaults on is that you can't. The default pre and post descriptions are gathered from information that is only available when pacman is run (which packages are installed, etc). Why not use pacnew for the example files?

Sorry but I do not understand at all what you mean? But you can provide a file with all options commented via # so it is a template.

wesbarnett commented 6 years ago

I could do that for snap-pac.conf, but I don't know how many snapper configurations a user has and wants to set up for the other configuration files. I personally prefer packages like that this to not provide the configuration file but instead just an example or a default one named with a different extension that can easily be copied or modified.

NicoHood commented 6 years ago

Sure, only for snap-pac.conf. The "config" configs are independent, for sure. Your choice. I will update the package now and you can decide at any time later.