Seite 1 von 1

Update Druckerinfos in CUPS und SAMBA inkl. snmp Abfrage

Verfasst: Montag 27. Juli 2009, 21:03
von Raziel
Hallo Zusammen.

Wir haben hier einen Linux Printserver (SLES10) der die Windows-Clients bedient. In den Druckern wurde der Standort der Geräte eingetragen. Um nun nicht die Infos per Hand in CUPS und SAMBA nachtragen zu müssen habe ich mir folgendes Script einfallen lassen. Die Drucker werden mit snmp abgefragt und per lpadmin und rpcclient werden die Infos im CUPS und SAMBA eingetragen.

Ich habe auf dem Server nur python 2.4.2 zur Verfügung und das python-cups Paket ist auch nicht installiert. Deshalb hab ich das ganze mit os.popen gelöst.

Was will ich nun von euch? Nun, ich wollte das ganze mal objektorientiert angehen, denn hab da noch keinerlei Erfahrung.
Hab ich das richtig umgesetzt oder sieht man an hand vom code, dass ich es nicht kapiert hab?
Was kann ich verbessern bzw. was hab ich falsch gemacht? (Ich weiss, ich hab viel zu wenig Kommentare reingemacht)


Danke schonmal im Vorraus für die Kritik bzw. Anregungen.

Code: Alles auswählen

#!/usr/bin/python

import os
import re
import time

from optparse import OptionParser

parser = OptionParser("getloc.py [Option] <Printer>")
parser.add_option("-l", "--list", dest="list", action="store_true",
help="show info about Printer via snmp")
parser.add_option("-u", "--updatecups", dest="updcups",
action="store_true", help="update CUPS info about Printer via snmp")
parser.add_option("-s", "--updatesamba", dest="updsamba",
action="store_true", help="update SAMBA info about Printer via snmp")
(options, args) = parser.parse_args()

# Defaults
def_Loc = ["xxx/ / "]
def_Desc = [" "]

# Class definitions
class printer(object):
   """ This class defines printer queues in CUPS and SAMBA.
       Defined methods and propertis:
          list()   -  lists information about the printers
          getLoc() -  gets the sysLocation from printer via snmpget
          getDesc()-  gets the hrDeviceDescr.1 from printer via snmpget
   """
   def __init__(self, queue):

       print "Initialize printer queue " + queue
       self.Queue = queue
       self.getLoc()
       self.getDesc()

   def list(self):
       print " "
       print "Printer Queue : " + self.Queue
       print "sysLocation   : " + self.Loc[0]
       print "sysDescription: " + self.Desc[0]

   def getLoc(self):
       # get sysLocation.0 via snmpget
       snmp_loc = os.popen("snmpget -v 1 -c public -O vq " + self.Queue
+ " sysLocation.0")
       self.Loc = [zeile.strip() for zeile in snmp_loc]
       rc = snmp_loc.close()
       # rc = None -> snmpget was successful
       if rc == None:
          print "--- sysLoc snmpget " + self.Queue + " successful "
          if self.Loc.count("") > 0:
             self.Loc.remove("")
          if len(self.Loc) == 0:
             print "--- Printer doesn't have snmp Location"
             self.Loc = def_Loc
       else:
          print "--- Error at sysLoc snmpget " + self.Queue
          self.Loc = def_Loc

   def getDesc(self):
       snmp_desc = os.popen("snmpget -v 1 -c public -O vq " + self.Queue
+ " hrDeviceDescr.1")
       self.Desc = [zeile.strip() for zeile in snmp_desc]
       rc = snmp_desc.close()
       if rc == None:
          print "--- Descr snmpget " + self.Queue + " successful "
          if self.Desc.count("") > 0:
             self.Desc.remove("")
          if len(self.Desc) == 0:
             print "--- Printer doesn't have snmp Description"
             self.Desc = def_Desc
       else:
          print "--- Error at Descr snmpget " + self.Queue
          self.Desc = def_Desc

   def updateCups(self):
       upd_cups = "/usr/sbin/lpadmin -p " + self.Queue + " -D\"" +
self.Loc[0] + " " + self.Desc[0] + "\""
       print upd_cups
       rc = os.system(upd_cups)
       if rc == 0:
          print "--- Update CUPS successful"
       else:
          print "--- Error by update CUPS"

   def updateSamba(self):
       upd_smb = "/usr/bin/rpcclient -k -c 'setprinter " + self.Queue +
" \"" + self.Loc[0] + " " + self.Desc[0] + "\"' localhost "
       print upd_smb
       rc = os.system(upd_smb)
       if rc == 0:
          print "--- Update SAMBA successful"
       else:
          print "--- Error by update SAMBA"

# Main

if __name__ == '__main__':
   printers = [printer(line) for line in args]
   if options.list:
      for i in range(len(args)):
          printers[i].list()
   if options.updcups:
      for i in range(len(args)):
          printers[i].list()
          printers[i].updateCups()
   if options.updsamba:
      for i in range(len(args)):
          printers[i].list()
          printers[i].updateSamba()

Verfasst: Montag 27. Juli 2009, 21:42
von cofi
Die Zeilen 10 - 17 sind unlesbar, da solltest du die jew. 2. Zeile einruecken (tritt aber auch noch oefters auf).
Die Klammern in Zeile 17 sind unnotig.
Und im ifmain brauchst du nicht ``range(len(args))`` vergewaltigen, sondern kannst direkt ueber ``printers`` iterieren.

Auf PEP8 solltest du auch mal einen Blick werfen.

Verfasst: Montag 27. Juli 2009, 22:23
von Leonidas
Außerdem würde man von einer Drucker-Klasse nicht erwarten, dass sie Ausgaben ``print``et sondern dass die Methoden die Ergebnisse der Berechnungen zurückgeben und sich jemand anders um die Ausgabe kümmert.

Verfasst: Dienstag 28. Juli 2009, 17:50
von Raziel
Danke für die Hinweise.
Ich habe mir PEP8 durgelesen und versuche das ganze jetzt zu korrigieren.
Die zu langen Zeilen habe ich umgebrochen und zwei daraus gemacht.
Die Method list() in der Printer-Klasse habe ich verworfen.
Dafür hab ich eine Funktion list() gemacht, der ich das Objekt printer übergeb.
Hier mache ich dann ein print printer.Loc.

Nun häng ich aber an folgenden Punkten:

1. Die Felder printer.Loc und printer.Desc sollen Defaultwerte erhalten wenn die Druckerabfrage kein Input bringt.
Dafür hab ich def_Loc und def_Desc gemacht.
Ist das so richtig oder gibts ne bessere variante? Eigentlich müsste das doch im init von der Printer Klasse gemacht werden, oder?

2. Mich stört noch das zusammensetzen der snmp und rpc-Befehle. Z.B. Der comuntiy String im snmp Befehl ist fest verdrahtet.
Das würde ich gerne über "Defaults" machen.
Nur wo definier ich die? Auch im Init?

3. Ist das Zusammensetzen des upd_cups, wie ich es gelöst habe korrekt oder gibts ein andere/bessere Vorgehensweise?

Danke für die Hilfe.

Verfasst: Dienstag 28. Juli 2009, 18:03
von cofi
Ersetze ``os.popen`` durch ``subprocess.Popen`` und den Befehl solltest du ueber eine Liste uebergeben.

Im uebrigen waers hilfreich, wenn du den neuen Code auch posten wuerdest ;)
Am besten auf paste.pocoo.org

Verfasst: Dienstag 28. Juli 2009, 21:15
von Raziel
so, hab das script hier http://paste.pocoo.org/show/131338/ gepostet.

Die print's in den funktionen der klasse Printer will ich noch entfernen, bzw. anders machen. die dienen eigentlich zur zeit nur als debug funktion/protokoll.

Probleme hab ich noch bei self.Loc und self.Desc. Diese sollen, falls der snmpget keine werte oder leere werte zurückliefert, einen Default Wert erhalten.
So wie ich das im Moment drin stehen habe, funktioniert es nicht, da der inhalt überschrieben wird.
Nur weiss ich nicht so recht, wo ich def_Loc (Defaultwert) deklarieren soll.

Und subprocess.Popen muss ich mir noch anschauen und einbauen.

Verfasst: Dienstag 28. Juli 2009, 21:39
von cofi
http://paste.pocoo.org/compare/131346/131338/

PEP 8 ist aber noch nicht ansatzweise durchgesetzt ;)

Verfasst: Dienstag 28. Juli 2009, 22:12
von Raziel
Gib mir mal nen tipp in Bezug auf PEP8.
Du meinst die Einrückung, oder?

Verfasst: Dienstag 28. Juli 2009, 22:20
von cofi
Nein ich meine die Bennung.
Die ``rc`` Tests brauchen uebrigens auch nicht den Zwischenschritt der Zuweisung.

Verfasst: Dienstag 28. Juli 2009, 22:24
von Raziel
Das hab ich mal so gelernt. Vor langer, langer Zeit.
Das es in Python ohne Zuweisung funktioniert, daran muss ich mich erst gewöhnen.

Verfasst: Dienstag 28. Juli 2009, 22:25
von EyDu
Raziel hat geschrieben:Gib mir mal nen tipp in Bezug auf PEP8.
Du meinst die Einrückung, oder?
get_desc
self.queue
self.desc
get_loc
loc
def_loc
def_desc

Noch ein paar Tips:
- String Formatting
- Konstanten bentuzen
- map(str.strip, snmp_loc)
- if rc is None

So mal als kleine Hinweise ^^