Bug 8214 - printer driver upgrade fails, causing smbd to exit on startup
Summary: printer driver upgrade fails, causing smbd to exit on startup
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Printing (show other bugs)
Version: 3.6.0rc2
Hardware: All All
: P5 regression
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 7803
  Show dependency treegraph
 
Reported: 2011-06-09 12:32 UTC by David Disseldorp
Modified: 2012-08-17 08:28 UTC (History)
4 users (show)

See Also:


Attachments
smbd log (827.48 KB, text/plain)
2011-06-09 12:58 UTC, David Disseldorp
no flags Details
proposed driver upgrade fix (3.96 KB, patch)
2011-06-09 23:08 UTC, David Disseldorp
no flags Details
driver upgrade fix using APD_COPY_FROM_DIRECTORY (14.83 KB, patch)
2011-06-27 20:28 UTC, David Disseldorp
no flags Details
v3-6-test patch (backport from master) (467.54 KB, patch)
2011-07-11 11:04 UTC, Guenther Deschner
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2011-06-09 12:32:09 UTC
Samba 3.6.0 offers a new spoolss architecture. Previously printer, driver and
form information was stored in three separate tdbs (ntprinting.tdb,
ntdrivers.tdb and ntforms.tdb). This information is now stored in Samba's
registry.

On spoolss initialisation nt_printing_tdb_migrate() is called to handle
migration from previous formats, it does this by issuing a AddPrinterDriver
request locally.

In processing the AddPrinterDriver spoolss request, clean_up_driver_struct() is
called which strips all directory components from driver file paths.

move_driver_file_to_download_area() then assumes that files corresponding to
the AddPrinterDriver request being handled are staged in the $architecture
subdirectory of the print$ share path. This is where clients upload printer
driver files prior to them being moved by the server into the
$architecture\$version directory.

As the driver files cannot be found (they are already in the
$architecture\$version directory), the AddPrinterDriver request fails resulting
in spoolss initialisation failure which in turn causes smbd to exit.
Comment 1 David Disseldorp 2011-06-09 12:58:21 UTC
Created attachment 6550 [details]
smbd log
Comment 2 David Disseldorp 2011-06-09 13:15:08 UTC
The approach I'm looking at is to add a new entry to spoolss_AddPrinterDriverExFlags (or hijack APD_DONT_COPY_FILES_TO_CLUSTER) which causes the driver file IO to be skipped in AddPrinterDriver processing.

Upgrading to blocker - failing to start smbd is disruptive.
Comment 3 David Disseldorp 2011-06-09 15:23:44 UTC
Proposed fix, further testing required:

The following changes since commit 314f161c00cfe3957f10b0f6f24adab737dfbe88:

  WHATSNEW: Update changes since rc1. (2011-06-07 20:13:47 +0200)

are available in the git repository at:
  git://oss.sgi.com/ddiss/samba 36s-bso8214-spoolss-drv

David Disseldorp (1):
      s3-printing: fix internal printing tdb upgrade

 librpc/idl/spoolss.idl                      |    3 ++-
 source3/printing/nt_printing_migrate.c      |   11 ++++++-----
 source3/rpc_server/spoolss/srv_spoolss_nt.c |   22 +++++++++++++---------
 3 files changed, 21 insertions(+), 15 deletions(-)

Feedback welcome.
Comment 4 Jeremy Allison 2011-06-09 21:25:50 UTC
Add the proposed fix as an attachment to this bug please !
Jeremy.
Comment 5 David Disseldorp 2011-06-09 23:08:41 UTC
Created attachment 6554 [details]
proposed driver upgrade fix
Comment 6 David Disseldorp 2011-06-09 23:14:37 UTC
(In reply to comment #4)
> Add the proposed fix as an attachment to this bug please !
> Jeremy.

Sorry Jeremy, please see attached.

The proposed change allows driver migration to successfully complete, however I'm now encountering problems in the printer migrate path. I'll raise a separate bug to track.
Comment 7 Karolin Seeger 2011-06-17 19:16:06 UTC
Re-assigning to Jeremy for patch review.
Comment 8 David Disseldorp 2011-06-20 09:16:51 UTC
Patch sent to samba-technical for review along with bug 8235 change:
http://lists.samba.org/archive/samba-technical/2011-June/078137.html
Comment 9 Jeremy Allison 2011-06-21 00:44:10 UTC
I'm sorry, I have to NAK this patch. We can't just grab a bit out of the spoolss_AddPrinterDriverExFlags bitmask and use it for our own purposes. Microsoft controls these values and can add their own flags at will. Can you fix this not to use a flag within this bitmask ? Is there somewhere else in this call we can flag this as internal ?

Jeremy.
Comment 10 Stefan Metzmacher 2011-06-21 07:01:50 UTC
I'd like use to share the code between source3/utils/net_printing.c and
source3/printing/nt_printing_migrate.c in future, as this is almost the same
code. So I would like to have this code working against windows and samba.

Wouldn't it make sense to upload the files (again) as a client would do?

Or experiment with the other APD_ flags.

Maybe we can use APD_COPY_FROM_DIRECTORY and specify the already existing
path names. If that also against windows, we can add a short-cut in our
code to skip them if the path is already the final one.

Or we try to implement APD_STRICT_UPGRADE and upload empty dummy files
with an older timestamp (If this also works against windows).
Comment 11 David Disseldorp 2011-06-21 09:55:43 UTC
(In reply to comment #9)
> Microsoft controls these values and can add their own flags at will. Can you
> fix this not to use a flag within this bitmask ? Is there somewhere else in
> this call we can flag this as internal ?

Metze's suggestion of using APD_COPY_FROM_DIRECTORY sounds good. We could use that alongside APD_COPY_NEW_FILES and convert migrate_driver() to issue a spoolss_AddDriverInfo6 request (rather than info3).

I'll see if I can get some test client code working against Windows and go from there.
Comment 12 Jeremy Allison 2011-06-22 18:44:13 UTC
Any update on this ? This is still set as blocker for 3.6.0.

Jeremy.
Comment 13 David Disseldorp 2011-06-22 19:11:32 UTC
(In reply to comment #12)
> Any update on this ? This is still set as blocker for 3.6.0.

Nothing yet, I'll work on this tomorrow.
Comment 14 Karolin Seeger 2011-06-24 19:40:56 UTC
Please note that the v3-6-test branch will be frozen on June 29.

Thanks!
Comment 15 David Disseldorp 2011-06-27 20:28:19 UTC
Created attachment 6639 [details]
driver upgrade fix using APD_COPY_FROM_DIRECTORY

APD_COPY_FROM_DIRECTORY has not yet been tested against a windows server so needs accompanying test code. For instance the fully qualified driver file paths specified with APD_COPY_FROM_DIRECTORY are interpreted by the Samba server as print$ specific paths which may be interpreted differently by Windows servers.
Comment 16 Karolin Seeger 2011-06-28 19:15:14 UTC
Re-assigning to Günther for patch review.
Comment 17 David Disseldorp 2011-06-29 08:36:34 UTC
(In reply to comment #15)
...
> APD_COPY_FROM_DIRECTORY has not yet been tested against a windows server so
> needs accompanying test code. For instance the fully qualified driver file
> paths specified with APD_COPY_FROM_DIRECTORY are interpreted by the Samba
> server as print$ specific paths which may be interpreted differently by Windows
> servers.

As discussed with Günther yesterday, Windows (tested against Server 2k8) interprets paths carried in AddPrinterDriverEx requests with APD_COPY_FROM_DIRECTORY as UNC share paths.
Comment 18 Karolin Seeger 2011-07-04 19:57:31 UTC
Günther,
please comment if this bug is still a blocker for the next 3.6 release.
Thanks!
Comment 19 David Disseldorp 2011-07-08 16:53:05 UTC
Günthers winreg based printing tdb migration changes are now in the tree, the same set of changes address this issue and bug 8235:

e595590 s3-printing: remove tdb migration invalid printer name checks
cfc3b6e s3-printing: make sure to first migrate the printers then the security
descriptor.
5dd8185 s3-printing: fill info2_mask in printer migration
57bbb32 s3-printing: remove spoolss pipe from migration library, only using
winreg finally.
8f3d5f5 s3-printing: use winreg interface for migration, instead of spoolss.
72b1f8b s3-printing: safe a ton of roundtrips by reusing existing winreg
binding_handles.
0a1ec73 s3-printing: use winreg_internal functions.
ada5380 s3-printing: add winreg_internal functions.
a762eda s3-printing: add winreg_printer_binding_handle and remove most of
srv_spoolss_util.c.
f2be837 s3-printing: add rpc_client/cli_winreg_spoolss.c
a0fc64a s3-waf: make LIBCLI_SPOOLSS a shared library
43cf3a2 s3-printing: move spoolss_create_default_devmode/secdesc to
init_spoolss.h
74e4160 s3-printing: move driver_info_ctr_to_info8 to init_spoolss.h
dd5375b s3-printing: move os2 related functions to printing/nt_printing_os2.c.
1caa7a8 s3-waf: Fix linking bugs causing segfaults.
ff94539 s3-printing: open up a winreg pipe handle for the migration code.
bafd721 s3-net: use printing_migrate library, and eliminate duplicate code.
e02abd6 s3-printing: split out printing migration code into a smaller library.
c9e3f6a s3-printing: skip migration of non-existent printers
a36ce07 s3-printing: fill devicemode size in migrate_printer()
Comment 20 Guenther Deschner 2011-07-11 11:04:06 UTC
Created attachment 6677 [details]
v3-6-test patch (backport from master)
Comment 21 Guenther Deschner 2011-07-12 10:33:29 UTC
Karolin, please add to 3.6.0
Comment 22 Guenther Deschner 2011-07-12 10:34:33 UTC
With this patchset, as David pointed out, this bug and bug #1194 are resolved
Comment 23 Karolin Seeger 2011-07-12 19:01:33 UTC
(In reply to comment #21)
> Karolin, please add to 3.6.0

Pushed to v3-6-test.
Closing out bug report.

Thanks!
Comment 24 Karolin Seeger 2011-07-12 19:04:11 UTC
(In reply to comment #22)
> With this patchset, as David pointed out, this bug and bug #1194 are resolved

I think bug #8235 is meant, right?