Project

General

Profile

Feature #3627

Add SMBmount handler

Added by davekempe about 6 years ago. Updated over 4 years ago.

Status:
Confirmed
Priority:
Low
Assignee:
-
Category:
new handler
Target version:
-
Start date:
11/14/2011
Due date:
% Done:

30%

QA Check:

Description

Hi,
we developed a handler for mounting windows shares.
Its important that the whole process stops if the windows share is not mounted correctly, as we didn't want rdiff backing up the whole directory.
I didn't actually write this handler, one of my ex-employees did, (I think it was Nicolas Brisac). So take it as it is, and if you like it, please feel free to include it in a newer version of backupninja

This is the handler:

$ cat smbmount.in
#!/bin/sh

  1. Mount a remote directory ('share') from a Samba server ('host')
  2. using ('user') and ('pass') to authenticate
  3. onto a destination directory ('mountpoint').
  1. ATTENTION
  1. This handler assumes that smbclient and smbfs packages are installed on the system

getconf share
getconf host
getconf user guest
getconf pass
getconf domain
getconf mountpoint

auth="${user:-}${pass:+%$pass}"

if [ ! -d "$mountpoint" ]; then
halt "Destination directory does not exist"
fi

if [ ! -n "`smbclient -N -U $auth -L $host | awk '{print $1}' | grep -xi $share`" ]; then
halt "Unable to connect to host '$host' or source share '$share' doesn't exist"
fi

if grep $mountpoint /proc/mounts; then
umount -f $mountpoint
fi

options="username=$user,password=$pass${domain:+,domain=$domain}"

mount -t cifs //$host/$share $mountpoint -o $options || halt "Mount failed"

and the config file looks like this:

share = sharename
host = windowshostname
user = userwithsharepermission
pass = passwordofuser

mountpoint = /mnt/mountpoint

So if that makes sense, please consider adding it to backupninja.
I have more stuff to contibute as well in another feature ticket.

History

#1 Updated by intrigeri almost 6 years ago

  • Category set to new handler
  • Status changed from New to In Progress
  • Assignee set to davekempe
  • % Done changed from 0 to 30

Hi!

Looks great, thank you for your contribution.

  • in the call to smbclient, $auth, $host etc. should be quoted
  • also missing quoting in the call to mount
  • capturing, and reporting back stdout / stderr is a must. Else it's impossible to debug.
  • if you don't write a ninjahelper and example configuration file, probably nobody will and this won't get merged.
  • The "is mounted" test using grep and /proc/mounts is not accurate enough and is thus fragile. Better use the mountpoint command which will take care of the details for you.

In any case, attaching a patch (or even better, publishing a Git branch) against current Git master branch would make reviewing much easier.

#2 Updated by intrigeri over 5 years ago

  • Priority changed from Normal to Low

Dave: ping?

#3 Updated by davekempe over 5 years ago

Hey,
sorry but I gave this job to Wes in my office and he hasn't had time to look at it. I will chase it up again.

#4 Updated by wes over 5 years ago

intrigeri wrote:

Dave: ping?

Hi intrigeri,

Regarding the ninjahelper for this, do you think it would be best to add smb share awareness to the rdiff, tar, (and so on) helpers directly?

Or should there be a separate smbmount helper that simply handles creating the mount action and then leave it up to the user to create an rdiff or tar action afterwards? (and then have a umount handler/helper action after that to clean up the samba mount?)

#5 Updated by intrigeri over 5 years ago

wes wrote:

Or should there be a separate smbmount helper that simply handles creating the mount action and then leave it up to the user to create an rdiff or tar action afterwards? (and then have a umount handler/helper action after that to clean up the samba mount?)

I prefer the dedicated SMB mount/umount handler/helper. A generic mount/umount handler/helper pair would be even better, so maybe call this mount even if it only supports SMB to start with.

#6 Updated by wes over 5 years ago

intrigeri wrote:

I prefer the dedicated SMB mount/umount handler/helper. A generic mount/umount handler/helper pair would be even better, so maybe call this mount even if it only supports SMB to start with.

No worries, should have this ready to submit soon, just need to finish the helper.

#7 Updated by wes over 5 years ago

Hi intrigeri,

I've pushed the initial mount/umount handler, helper, and example configs to https://github.com/sol1/backupninja.git under the 'mount' branch

#8 Updated by intrigeri over 5 years ago

  • Assignee changed from davekempe to intrigeri

#9 Updated by intrigeri over 5 years ago

wes wrote:

I've pushed the initial mount/umount handler, helper, and example configs to https://github.com/sol1/backupninja.git under the 'mount' branch

Great! I'll try to review it soon. Don't hesitate nagging me every two weeks if needed.

#10 Updated by intrigeri over 5 years ago

wes wrote:

I've pushed the initial mount/umount handler, helper, and example configs to https://github.com/sol1/backupninja.git under the 'mount' branch

First part of my review (not read everything yet):

  • example.mount must be well commented
  • I suggest adding a type parameter, only support type = smb to start with, but require it to be set in the config file => for future extensibility.
  • quoting $REPLY, $mountpoint and other variables in the helper is not optional
  • I don't think it's the helper's job to create the mountpoint, because one may setup a mount job without using the helper. The handler should, well, handle it.
  • Defaulting to a filename like 90_$host-$share.umount without validating the chars in there is scary. Yeah, I guess other helpers contributed in the past perhaps do the same too, but at some point we have to raise the bar a bit.
  • The handler is missing the editors modeline.
  • The handler has a useless shebang.

#11 Updated by intrigeri over 5 years ago

  • Assignee changed from intrigeri to davekempe

Some more review:

  • I think that failure to unmount, and trying to umount lazily, should be reported as a fatal error, using fatal, instead of being hidden in a warning that most admins won't see.
  • The umount handler also has a useless shebang and lacks editors modeline.
  • The doc must make it clear any error in the mount handler is a fatal one, that will block all further backup actions from being attempted.
  • git grep require_packages will enlighten you (a bit :)

#12 Updated by davekempe over 4 years ago

Hey
If it wasn't obvious before, I'm sorry but I don't have time to update this code for some time to come. If anyone wants to take this stuff and run with it, please go for it.

#13 Updated by intrigeri over 4 years ago

  • Status changed from In Progress to Confirmed
  • Assignee deleted (davekempe)

Also available in: Atom PDF