redesign shared folder handling

[hgy59]

Description

The volume specified in the install wizard for shared folders is not used on DSM 7 (and on DSM 6, when using the resource worker with USE_DATA_SHARE_WORKER = yes)

By default the shared folders created by DSM are located in the same volume as the package is installed to.
It is possible to create shared folders on a different volume and use an existing share in the installation wizard.
This is currently not supported by our framework (at least not for DSM 6 when using the resource worker)

There is some code in the framework, that handles the volume chosen in the wizard, but only when the variable is named wizard_volume.
This is undocumented and bogus, and will be removed here. It is replaced by evaluation of the full path of the share by the system function synoshare on DSM 6 and by using the provided link under shares on DSM 7 (non root user is denied to call synoshare)

Related to #5481
Fixes #5041

Remarks

This got more framework fixes than originally planned.

Finally this PR updates the demoservice package only and introduces the Makefile variable SERVICE_WIZARD_SHARENAME to not affect existing packages that use shared folders the old way (i.e. SERVICE_WIZARD_SHARE and optionally USE_DATA_SHARE_WORKER).

While testing the new shared folders with demoservice, an issue with SERVICE_COMMAND on SRM and DSM5 popped up. Therefore a dedicated start script without parameters is used in demoservice now. SERVICE_COMMAND with parameters does not work together with SVC_BACKGROUND=y and SVC_CREATE_PID=y on platforms not supporting bash shell (i.e. ash).

Another issue newly discoverd is, that load_variables_from_file does not work to load installer variables. This is fixed by direct call of the function (to avoid a sub shell by use of call_func).
And load_variables_from_file had an issue on SRM and DSM5 (cut with parameter --delimiter is not supported there)

Another issue affects the installer variables persisted in etc/installer-variables. This file was removed on package updates without an upgrade wizard. Without an upgrade wizard that activly reloads the variables into the wizard before the upgrade, those variables are not defined anymore. The fix avoids the deletion of the installer-variables file in such cases.

Additionally this PR completly removes the (deprecated) support for SERVICE_EXE and SERVICE_WIZARD_GROUP.

And one more issue fixed. The DSM 7 uninstaller was not able to delete the files in the package etc folder. This was also a security issue, since the installer-variables file might contain sensitive information.

And (hopefully) a final fix: On DSM 7, when a package is uninstalled without removing the data, the etc/installer-variables file already exists when the package is installed again. To avoid that wizard variables get overwritten by installer-variables, the function load_variables_from_file now ignores variables that are already defined. Otherwise the shared folder name (and path) was always taken from installer-variables and not from the wizard.

To create a shared folder by a package, there must exist a service user. to give access to the folder. On packages for DSM <7, SERVICE_USER = auto must be defined. This is now enforced, when SERVICE_WIZARD_SHARENAME is defined.
And #5041 is now fixed by implicit definition of SERVICE_USER = auto in spksrc.service.mk for DSM 7.
Probably we should remove the need to define SERVICE_USER = auto at all, and always use a package user (sc-packagename). For the case that on DSM < 7 a service user must not be defined, we could add a variable to define NO_SERVICE_USER = y. But this definitivly goes beyond the scope of this PR...

Overview: Packages with shared folder

Since this PR does not affect existing packages anymore, the following packages can be migrated one by one after this PR is merged.

Package	Variable	DSM 7	DSM 6 resource worker	DSM 6 old	DSM 5	SRM	Remarks
aria2	wizard_download_dir						not published
deluge	wizard_download_dir	x		x
demoservice	wizard_download_dir						noarch package, not published
gitea	wizard_gitea_dir	x	x		x	x
google-fonts	wizard_shared_folder	x	x				too large to publish
kiwix	wizard_data_folder	x	x			x
minio	wizard_data_directory	x	x		x	x
mpd	wizard_music_folder	x	x			x
nzbget	wizard_download_folder	x	x		x		noarch package
owncloud	wizard_data_share	x	x				noarch package
ps3netsrv	wizard_data_directory	x		x	x	x
rutorrent	wizard_download_dir	x		x		x
sabnzbd	wizard_download_dir	x		x
transmission v3	wizard_download_dir	x		x	x	x
transmission v4	wizard_download_share	x					DSM 7 only

Affected PRs

• add Urbackup #5213
• subversion: initial package #5296
• navidrome: initial package release #5376
• restic rest-server 0.12.1 initial package #5550
• update minio to 2022-10-24T18-35-07Z #5555
• Transmission: Update to v4.0.2 #5616
• rutorrent: upgrade 3.10 => 4.1.5 #5617
• ownCloud: Update to version 10.12 #5619 (should use a shared folder for wizard_owncloud_datadirectory)
• Update Gitea to v1.18.5 #5624
• mpd: update to v0.23.15 #5652

Checklist

• Build rule all-supported completed successfully
• New installation of package completed successfully
• Package upgrade completed successfully (Manually install the package again)
• Package functionality was tested
• Any needed documentation is updated/created

Type of change

• Includes small framework changes

[publicarray]

I'm probably missing something but I remember working on the ability to choose a volume for downloads with the transmission package using the resource worker, not sure if this is relevant here. It was the only package that I know of that fully made use of it and coursed some issues with how a shared folder was created. Something might have changed since then or I never documented it fully.

[th0ma7]

@publicarray indeed, something also used for deluge package.

[hgy59]

@publicarray @th0ma7 I tend to introduce new Makefile variables for this. The migration of existing packages is a mess, as it depends on multiple facts

currently:

• packages with wizard_volume as variable for the volume chosen in the wiazrd have currently full SHARE_PATH but the others have only the share name as SHARE_PATH
• SHARE_PATH is perstisted in the installer-variables for some packages, but not for all
• packages that provide to update the shared path in the upgrade wizard need to reconstruct the share name from the installer variables
• not all packages use shared folder resources for DSM 6

future:
we should provide two installer-variables for shared folders

• one for the full path (required by the packages to access the folder)
• one for the name of the share (optional for upgrade wizards)

[publicarray]

Would it be easier to have a script that just splits the path string into the variables (volume, share) and combine them when needed (path)?

[publicarray]

Never mind I remember now that the resource worker requires them to be seperate (volume, share)

[hgy59]

Never mind I remember now that the resource worker requires them to be seperate (volume, share)

No, the resource workes takes the (folder) name only. The volume cannot be specified with a resource worker.
The volume is only needed for DSM 5 (and DSM 6 without resource worker) when the installer code creates the share with synoshare.

We already have such code, that splits the SHARE_PATH into SHARE_VOLUME and SHARE_NAME. But this is not usable.
With resource workes, we have the name only and must evaluate the full path.

[th0ma7]

I haven't touched or played much with this side of the framework. I won't be able to provide much assistance on this 🤷

[publicarray]

Thanks for cleaning that up. I like what you proposed.

[mreid-tt]

hey @hgy59, I attempted to include this PR as part of a revision to the Transmission build #5616. In selecting volume2 as my volume and torrents as my share path the installation failed and no share was created. I've included the following logs for your review:

• https://pastebin.com/SZxA5eCT (transmission.log)
• https://pastebin.com/h8h4PLjG (synopkg.log)

Let me know if there is anything I can do to assist with making this fix work. From my other tests, creating a folder on volume2 in the base package (before this PR), it also fails with log entries like this:

mkdir: cannot create directory '/volume2/torrents': Permission denied

Checking the Developer Guide Data Share resource worker documentation as well as your earlier comments it doesn't seem possible to assign a volume argument for creating a share. As such, I'm not sure what is the overall solution we are working toward.

[mreid-tt]

hey @hgy59, I did a few more tests with Transmission 4 and it seems that the Makefile must still only refer to the share name and not the full path. Using the full path (including volume) in the generated conf/resource file results in the share not being created.

This however does not prevent other errors (like the mkdir ones above) from occurring. I've since rolled back testing of this patch from the Transmission PR and instead taken some of the logic. I included a check for the existence of the full path and if not valid, use the /var/packages/[PKG]/shares/[SHARE] to resolve the real path. That way the solution would work in both incorrect or correct volume situations (share not on requested volume or is a manually created share).

[hgy59]

hey @hgy59, I did a few more tests with Transmission 4 and it seems that the Makefile must still only refer to the share name and not the full path. Using the full path (including volume) in the generated conf/resource file results in the share not being created.

This however does not prevent other errors (like the mkdir ones above) from occurring. I've since rolled back testing of this patch from the Transmission PR and instead taken some of the logic. I included a check for the existence of the full path and if not valid, use the /var/packages/[PKG]/shares/[SHARE] to resolve the real path. That way the solution would work in both incorrect or correct volume situations (share not on requested volume or is a manually created share).

Yes that's what I am trying to fix/explain here: for shared folders, a wizard should never ask for a volume but for the share name only.
As the packages need the full name of the shared folder we have to evaluate the full path (including the volume).
EDIT: and we must not check for the existance of a shared folder (at least not in preinst).

And when we use the USE_DATA_SHARE_WORKER = yes we never must create a shared folder by the installer (except for DSM 5, if we ever have packages for DSM 5 with a shared folder).

BTW synology provides the variable SYNOPKG_PKGDEST_VOL with the volume where the package is installed to (OK, you already found this in #5619)

[mreid-tt]

Hmm, okay. Perhaps I don't understand how this PR works then. Would you say that the main objectives are:

1. The SERVICE_WIZARD_SHARE should expect a share name at all times and not a full path (i.e. without a volume)
2. Remove undocumented logic for handling the wizard_volume variable
3. For the SERVICE_WIZARD_SHARE, evaluate the full path (including volume) to be used elsewhere in the framework

If this understanding is correct, then it seems to be checking for the share before it is created based on the logs. And when we use just the share name to create the shared folder, what variable should we be using in scripts to derive the full path? Apologies if I'm still not understanding correctly.

[hgy59]

@mreid-tt sorry, this is still WIP. I started to introduce new variables and not to touch existing packages (locally, not pushed yet). And I did not yet update any existing package that will show the final solution...

[fgma]

Whats the status here? I'd like to update my minio installation to a version without any critical security issues,

[Safihre]

Related to #5722, since it as been broken for ages, do we really need to postpone updates because of this? It's not like this was broken recently and it also only affects users that want to use a non-default spot.
I personally doubt if fixing this outweigh the benefit of updates to the applications.

[fgma]

Any news here? What is missing? Can we provide any help to get this done? If this blocks so many other packages this should have a high priority.

[fgma]

Can we please get any reaction on this? My system is running out of space and needs an upgrade ASAP which is the only chance for me switching to the new minio data format.

[hgy59]

@fgma you can continue using the existing approach for shared folders.

I will try to create a migration path for this redesign with new Makefile variable(s) to not touch existing packages.

[fgma]

@hgy59 As far as I understood the issue here my migration will have a problem because I'm going to add a second disk to my system without using raid. The workaround of creating the share manually before installing the package might work for me.

The other issue is the final migration path to the new minio data format. I'd like to update my system ASAP but in the end continue using future minio SPKs.

[publicarray]

Thanks I'll double check. It's probably because I'm not on the latest dem6 version on my test VM.

[hgy59]

Thanks I'll double check. It's probably because I'm not on the latest dem6 version on my test VM.

The synology dev guide documents that data-share resource workers are supported since DSM 6.0-5914. Our DSM 6 noarch packages define TC_OS_MIN_VER = 6.1-15047 so this should be supported on almost all available DSM 6 versions.

Alternativly you could install the noarch all package (TC_OS_MIN_VER = 3.1-1594). But this one does not use the resource worker to create shared folders.

[mreid-tt]

@hgy59 regarding b6acead, does this replace the change I made a while back in 1b0cb80 for icon creation?

[hgy59]

@hgy59 regarding b6acead, does this replace the change I made a while back in 1b0cb80 for icon creation?

No, it does not replace it, it enhances it to create icons for web service packages (that define DSM_UI_CONFIG but not SERVICE_PORT nor ADMIN_URL)

Currently only cops has a workaround to fix this (by definition of ICON_DIR = $(STAGING_DIR)/$(DSM_UI_DIR)/images)

Other packages (owncloud, rutorrent, memcached) have defined ADMIN_URL to get icons in the package. (in fact, this is a workaround too, since ADMIN_URL is not required for packages that create an icon in DSM UI).

[mreid-tt]

@hgy59 this seems to be related... https://github.com/SynoCommunity/spksrc/projects/9#card-62758887. should it be moved to done?