Opened 13 years ago

Closed 13 years ago

#2715 closed defect (fixed)

ValueError in mythburn.py due to improper parsing of "mount" command output

Reported by: eliasen@… Owned by: paulh
Priority: major Milestone: unknown
Component: mytharchive Version: 0.20
Severity: high Keywords:
Cc: Ticket locked: no

Description

When attempting to write over a DVD+RW, mythburn.py fails with the following message:

ValueError?: too many values to unpack

I traced this to the following condition:

In the function copyRemote(files,tmpPath), a pipe is opened to the "mount" command and its output is split using the following code:

    # What does mount say?
    mounts = os.popen('mount')
    # Go through each line of mounts output
    for line in mounts.readlines():
        write(line)
        parts = line.split()
        # mount says in this format
        device, txt1, mountpoint, txt2, filesystem, options = parts

The error occurs in the last line. I added some write statements to see what was going on and found that mount was producing the following output:

/dev/mapper/VolGroup00-LogVol00 on / type ext3 (rw)
proc on /proc type proc (rw)
sysfs on /sys type sysfs (rw)
devpts on /dev/pts type devpts (rw,gid=5,mode=620)
/dev/hda2 on /boot type ext3 (rw)
tmpfs on /dev/shm type tmpfs (rw)
none on /proc/sys/fs/binfmt_misc type binfmt_misc (rw)
sunrpc on /var/lib/nfs/rpc_pipefs type rpc_pipefs (rw)
//192.168.1.4/mp3 on /home/eliasen/mnt/mp3 type cifs (rw,mand)
//clemens/games on /home/eliasen/mnt/games type cifs (rw,mand)
//clemens/cvs on /home/eliasen/mnt/cvs type cifs (rw,mand)
/dev/hdc on /media/MythTV BurnDVD type udf (rw,noexec,nosuid,nodev,uid=500)

The error was produced when processing the last line. Note that the name of the last mount point is "/media/MythTV BurnDVD" which includes a space. (This is a name that mytharchive chose by default, so mytharchive produces the condition that causes it to produce errors.)

This is a DVD+RW drive that I had previously (successfully) recorded mythtv output onto, and was attempting to write over. However, since the splitting of the line is rather naive, and simply splits on blanks, it can not handle a space in the name of the directory, and breaks. The DVD cannot be written.

A workaround might be for the user to manually blank the DVD or give it another volume title with another program before attempting to write over it, using a command like:

dvd+rw-format -blank /dev/dvd

Another (weak) workaround that would help people in the short term might be to change the defaults so that the name of the volume produced by mytharchive didn't have a space in it. This might help a lot of scripts that parse mount points and directory names and don't properly quote fields.

The real fix will be to change the way that mount points are fetched or parsed. Parsing the output of "mount" may be brittle and sensitive to the version of mount. I don't readily see a way to iterate through mount points in Python, so we may just need to fix the parsing.

I submit the following as a potential patch, using a regular expression to parse the output of mount, but I don't know if it's reasonable. The problem might be that "mount" uses different delimiter words in different locales, which will of course break this parsing. The matches for the "device" and "mountpoint" fields are greedy, so they'll match up to the first " on " and " type " respectively, meaning that both fields could contain spaces.

*** mythburn.py.orig	2006-11-21 23:14:43.000000000 -0700
--- mythburn.py	2006-11-22 00:52:18.000000000 -0700
***************
*** 52,57 ****
--- 52,58 ----
  import Image, ImageDraw, ImageFont
  import MySQLdb, codecs
  import time, datetime, tempfile
+ import re
  from fcntl import ioctl
  from CDROM import CDROMEJECT
  from CDROM import CDROMCLOSETRAY
***************
*** 1206,1212 ****
      return (width, height)
  
  def runMythtranscode(chanid, starttime, destination, usecutlist, localfile):
!     """Use mythtrancode to cut commercials and/or clean up an mpeg2 file"""
  
      if localfile != "":
          localfile = quoteFilename(localfile)
--- 1207,1213 ----
      return (width, height)
  
  def runMythtranscode(chanid, starttime, destination, usecutlist, localfile):
!     """Use mythtranscode to cut commercials and/or clean up an mpeg2 file"""
  
      if localfile != "":
          localfile = quoteFilename(localfile)
***************
*** 3239,3251 ****
      # Define remote filesystems
      remotefs = ['nfs','smbfs']
      remotemounts = []
      # What does mount say?
      mounts = os.popen('mount')
      # Go through each line of mounts output
      for line in mounts.readlines():
!         parts = line.split()
!         # mount says in this format
!         device, txt1, mountpoint, txt2, filesystem, options = parts
          # only do if really remote
          if filesystem in remotefs:
              # add remote to list
--- 3240,3258 ----
      # Define remote filesystems
      remotefs = ['nfs','smbfs']
      remotemounts = []
+     mountPattern =  re.compile(r"^(.*?)\s+on\s+(.*?)\s+type\s+(\S+)\s+(.*)")
      # What does mount say?
      mounts = os.popen('mount')
      # Go through each line of mounts output
      for line in mounts.readlines():
!         match = mountPattern.search(line)
!         if match:
!            device, mountpoint, filesystem, options = match.group(1,2,3,4)
!         else:
!             write("Unmatched line in mount output:")
!             write(line)
!             continue
! 
          # only do if really remote
          if filesystem in remotefs:
              # add remote to list

This patch should be treated with some skepticism.

--

Alan Eliasen eliasen@…

Change History (3)

comment:1 Changed 13 years ago by anonymous

I thought of another solution that's probably a lot more portable. The python method os.ismount() will return true if a particular directory is a mount point. When we get a path, to determine if it's remotely mounted, we'd just have to repeatedly call os.split() on the pathname and os.ismount() on the remaining head of the path to see if any of the directories above are actually mount points. This would not be excessively slow (probably quicker than forking mount), and should be portable.

I haven't actually written a patch that does this, so if the person responsible could indicate their progress on this bug, I can write one up.

--

Alan Eliasen eliasen@…

comment:2 in reply to:  1 Changed 13 years ago by paulh

Replying to anonymous:

I thought of another solution that's probably a lot more portable. The python method os.ismount() will return true if a particular directory is a mount point. When we get a path, to determine if it's remotely mounted, we'd just have to repeatedly call os.split() on the pathname and os.ismount() on the remaining head of the path to see if any of the directories above are actually mount points. This would not be excessively slow (probably quicker than forking mount), and should be portable.

I haven't actually written a patch that does this, so if the person responsible could indicate their progress on this bug, I can write one up.

--

Alan Eliasen eliasen@…

Unless I'm misunderstanding something I don't think that would work. It would only tell you that you've reached a mount point not whether it is a moint point on a remote FS which is what we want.

It's a shame that python doesn't have something like os.statfs() which would make it easy.

It should be easy to do something like this in c so I might just end up adding an option in mytharchivehelper that the python script can call to determine if a file is on a remote FS.

comment:3 Changed 13 years ago by paulh

Resolution: fixed
Status: newclosed

(In [12202]) Change how mythburn.py detects whether a file is on a remote filesystem.

The current method of parsing the output of the mount command is a little fragile and falls over when a mount has a space in the name. It would be better to use statfs to determine if a file is on a remote fs except that python doesn't seem to have an equivalent of the 'c' statfs function.

This commit just adds a new function to mytharchivehelper that the script can call to do the check.

Fixes #2715.

Note: See TracTickets for help on using tickets.