Bug 8719 - printing fails in function cups_job_submit
Summary: printing fails in function cups_job_submit
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Printing (show other bugs)
Version: 3.6.1
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-25 16:02 UTC by Stefan Winter
Modified: 2012-07-24 18:54 UTC (History)
2 users (show)

See Also:


Attachments
Rebase of comment#4 changes for 3.6-test (77.86 KB, patch)
2012-06-25 17:42 UTC, David Disseldorp
no flags Details
Updated 3.6-test patchset (78.55 KB, patch)
2012-06-27 15:48 UTC, David Disseldorp
ddiss: review? (gd)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Winter 2012-01-25 16:02:33 UTC
This is after an upgrade from 3.5.5 to 3.6.1 on the same base system. With 3.6.1, printing fails with the following log message in log.smbd

adminpc-rmarx (158.64.1.188) connect to service SuperJam PCL6 initially as user Administrator (uid=1003, gid=100) (pid 15885)
[2012/01/25 13:14:01.334500,  2] rpc_client/cli_winreg_spoolss.c:898(winreg_create_printer)
  winreg_create_printer: Skipping, SOFTWARE\Microsoft\Windows NT\CurrentVersion\Print\Printers\SuperJam PCL6 already exists
[2012/01/25 13:14:01.575723,  0] printing/print_cups.c:940(cups_job_submit)
  cups_job_submit: failed to parse jobid from name /usr/local/samba/var/spool/samba/smbprn..CXRIP3
[2012/01/25 13:14:11.775067,  1] smbd/service.c:1291(close_cnum)
  adminpc-rmarx (158.64.1.188) closed connection to service SuperJam PCL6

Looking into the spool directory, the file names which used to encode the jobid now don't any more:

-rw------- 1 rmarx      users       0 Jan 25 08:46 smbprn..zpuUmw
-rw------- 1 rmarx      users       0 Jan 25 15:25 smbprn..zqdI3C
-rw------- 1 rmarx      users       0 Jan 25 15:09 smbprn..zqu3kj
-rw------- 1 rmarx      users       0 Jan 25 14:46 smbprn..zx3GuR
-rw------- 1 rmarx      users       0 Jan 25 15:28 smbprn..zxNUxy

(and so on)

After discussing this issue on the samba mailing list on 25 jan 2012, I was told that this appears to be a bug, hence the bug report here. The responder on the mailing list stated:

"The spoolss print job file open code-path has changed recently, looking
at print_spool_open() it looks like the job file is created without
taking the print jobid into account."

Greetings,

Stefan Winter
Comment 1 David Disseldorp 2012-01-25 18:34:39 UTC
Currently when smbd takes responsibility for spooling a print job, the spool-file path is generated and created in print_spool_open() without a jobid component.
StartDocPrinter is then called to register the corresponding job with the spoolss layer. The spoolss layer validates the spool-file's existence.

When EndDocPrinter is called, the spool-file path is passed to cups_job_submit() for printing. cups_job_submit() uses this path to obtain the print jobid via  print_parse_jobid(), which assumes a smbprn.$JOBID.$TMP_HASH format. Where the spool-file path does not meet this format requirement job submission fails as per the initial report. Where the format requirement is met, the jobid is used in constructing a value for the cups "job-name" attribute.

Obtaining the spoolss jobid from the spool-file path should be unnecessary - the cups layer should be agnostic as to format of the spool-file path. Furthermore a jobid can be passed to cups from the caller via struct printjob.

This leaves two other callers of print_parse_jobid() in the printer queue processing code-path, which require a similar fix before the function can be removed.
Comment 2 David Disseldorp 2012-02-09 17:03:04 UTC
Hi Stefan,

I've got a patch set together which largely changes how print jobs are tracked between spoolss and the underlying print backend. If you're able to test from the git repo below that would be helpful, I could also provide a single patch or openSUSE based RPMs if you would prefer.

In the mean time I'll continue writing smbtorture tests.

The following changes since commit 9d5ed16ddac1598918338a432e9effa8ab869300:

  s3:winbindd fix a return code check (2012-02-03 20:39:24 +0100)

are available in the git repository at:
  git://git.samba.org/ddiss/samba.git bso8632_print_jobid_tracking_rb2

David Disseldorp (17):
      s3-printing: store print jobid as part of struct printjob
      s3-printing: remove print_parse_jobid() from print_cups.c
      s3-printing: rename queue->job sysjob
      s3-printing: remove print_parse_jobid() calls from printing.c
      s3-printing: remove redundant variable set
      s3-printing: remove print_parse_jobid()
      s3-spoolss: remove duplicate "." in smbd spooler path
      torture: RemoteFindFirstPrinterChangeNotifyEx flags
      torture: test spoolss job notifications
      s3-printing: fix potential print db refcount leak
      s3-printing: clean up print_job_pause/resume interface
      s3-printing: return talloced print jobs
      s3-printing: pass a talloc ctx to unpack_pjob
      s3-printing: remove unused print_job_fname()
      s3-printing: pass lpq command to job_submit
      s3-printing: fill print_generic sysjob id on job submission
      torture: run DCE/RPC server across spoolss notify suite

 source3/include/printing.h                  |   23 +-
 source3/printing/lpq_parse.c                |   41 +--
 source3/printing/print_cups.c               |   18 +-
 source3/printing/print_generic.c            |  149 +++++----
 source3/printing/print_iprint.c             |    6 +-
 source3/printing/printing.c                 |  483 +++++++++++++++++----------
 source3/printing/printspoolss.c             |   11 +-
 source3/rpc_server/spoolss/srv_spoolss_nt.c |   30 +-
 source4/torture/rpc/spoolss.c               |    3 +-
 source4/torture/rpc/spoolss_notify.c        |  298 +++++++++++++++--
 source4/torture/rpc/spoolss_test.h          |   28 ++
 11 files changed, 746 insertions(+), 344 deletions(-)
 create mode 100644 source4/torture/rpc/spoolss_test.h
Comment 3 Stefan Winter 2012-02-10 07:02:24 UTC
Hi,

thanks, looks like fixing this is quite some work!

Is it possible to get a patch against 3.6.1 (preferably) or 3.6.3 tar source? That would make it easier to test for us - the system is in production and I don't feel comfortable fiddling with a unreleased GIT version ...

Greetings,

Stefan
Comment 4 David Disseldorp 2012-06-25 17:38:31 UTC
This patch-set has been proposed upstream...

http://gitweb.samba.org/?p=ddiss/samba.git;a=shortlog;h=refs/heads/bso8719_smbd_spooler_rb1

The following changes since commit c983ea8e5dc30111f6b8407307c3212635593949:

  s4-join: Setup correct DNS configuration (2012-06-24 18:10:10 +0200)

are available in the git repository at:
  git://git.samba.org/ddiss/samba.git bso8719_smbd_spooler_rb1

David Disseldorp (17):
      torture: add test for smbd print job spooling
      s3-printing: store print jobid as part of struct printjob
      s3-printing: remove print_parse_jobid() from print_cups.c
      s3-printing: rename queue->job sysjob
      s3-printing: remove print_parse_jobid() calls from printing.c
      s3-printing: remove redundant variable set
      s3-printing: remove print_parse_jobid()
      s3-spoolss: remove duplicate "." in smbd spooler path
      s3-printing: fix potential print db refcount leak
      s3-printing: clean up print_job_pause/resume interface
      s3-printing: return talloced print jobs
      s3-printing: pass a talloc ctx to unpack_pjob
      s3-printing: remove unused print_job_fname()
      s3-printing: pass lpq command to job_submit
      s3-printing: fill print_generic sysjob id on job submission
      s3-printing: use euid for vlp job tracking
      s3-torture: Use static printer for smbd spooler test

 source3/include/printing.h                  |   23 +-
 source3/printing/lpq_parse.c                |   41 +--
 source3/printing/print_cups.c               |   18 +-
 source3/printing/print_generic.c            |  149 +++++---
 source3/printing/print_iprint.c             |    6 +-
 source3/printing/printing.c                 |  505 +++++++++++++++++----------
 source3/printing/printspoolss.c             |   11 +-
 source3/printing/tests/vlp.c                |    3 +-
 source3/rpc_server/spoolss/srv_spoolss_nt.c |   30 +-
 source4/torture/rpc/spoolss.c               |   81 +++++
 10 files changed, 546 insertions(+), 321 deletions(-)
Comment 5 David Disseldorp 2012-06-25 17:42:00 UTC
Created attachment 7670 [details]
Rebase of comment#4 changes for 3.6-test

Rebase of comment#4 changes for 3.6-test...

http://gitweb.samba.org/?p=ddiss/samba.git;a=shortlog;h=refs/heads/bso8719_smbd_spooler_rb1_36t

The following changes since commit dd4a51406cc5789e984aab4861e9276448fccf60:

  WHATSNEW: Break line properly. (2012-06-24 19:19:19 +0200)

are available in the git repository at:
  git://git.samba.org/ddiss/samba.git bso8719_smbd_spooler_rb1_36t

David Disseldorp (17):
      torture: add test for smbd print job spooling
      s3-printing: store print jobid as part of struct printjob
      s3-printing: remove print_parse_jobid() from print_cups.c
      s3-printing: rename queue->job sysjob
      s3-printing: remove print_parse_jobid() calls from printing.c
      s3-printing: remove redundant variable set
      s3-printing: remove print_parse_jobid()
      s3-spoolss: remove duplicate "." in smbd spooler path
      s3-printing: fix potential print db refcount leak
      s3-printing: clean up print_job_pause/resume interface
      s3-printing: return talloced print jobs
      s3-printing: pass a talloc ctx to unpack_pjob
      s3-printing: remove unused print_job_fname()
      s3-printing: pass lpq command to job_submit
      s3-printing: fill print_generic sysjob id on job submission
      s3-printing: use euid for vlp job tracking
      s3-torture: Use static printer for smbd spooler test

 source3/include/printing.h                  |   23 +-
 source3/printing/lpq_parse.c                |   41 +--
 source3/printing/print_cups.c               |   18 +-
 source3/printing/print_generic.c            |  149 +++++---
 source3/printing/print_iprint.c             |    6 +-
 source3/printing/printing.c                 |  505 +++++++++++++++++----------
 source3/printing/printspoolss.c             |   11 +-
 source3/printing/tests/vlp.c                |    3 +-
 source3/rpc_server/spoolss/srv_spoolss_nt.c |   30 +-
 source4/torture/rpc/spoolss.c               |   80 +++++
 10 files changed, 545 insertions(+), 321 deletions(-)
Comment 6 David Disseldorp 2012-06-27 15:48:55 UTC
Created attachment 7673 [details]
Updated 3.6-test patchset

The following commit is needed on top of the previous 3.6-test patch-set:

The following changes since commit 04f2d2e4e97463a8aeab3ba5197d5d9efbfabf0f:

  s3-torture: Use static printer for smbd spooler test (2012-06-25 15:26:11 +0200)

are available in the git repository at:
  git://git.samba.org/ddiss/samba.git bso8719_smbd_spooler_rb1_36t

David Disseldorp (1):
      s3-printing: fix broken print_job_get_name() return

 source3/printing/printing.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Comment 7 Jeremy Allison 2012-07-20 23:37:51 UTC
+1 on this patchset for 3.6.next.

Jeremy.
Comment 8 Jeremy Allison 2012-07-20 23:52:31 UTC
Re-assigning to Karolin for inclusion in 3.6.next.
Jeremy.
Comment 9 Karolin Seeger 2012-07-21 18:44:21 UTC
(In reply to comment #6)
> Created attachment 7673 [details]
> Updated 3.6-test patchset
> 
> The following commit is needed on top of the previous 3.6-test patch-set:
> 
> The following changes since commit 04f2d2e4e97463a8aeab3ba5197d5d9efbfabf0f:
> 
>   s3-torture: Use static printer for smbd spooler test (2012-06-25 15:26:11
> +0200)
> 
> are available in the git repository at:
>   git://git.samba.org/ddiss/samba.git bso8719_smbd_spooler_rb1_36t
> 
> David Disseldorp (1):
>       s3-printing: fix broken print_job_get_name() return
> 
>  source3/printing/printing.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Hi David,

can you provide the complete patchset, please?

Thanks!

Karolin
Comment 10 Jeremy Allison 2012-07-23 17:22:22 UTC
Karolin, despite David's message this attachment:

https://attachments.samba.org/attachment.cgi?id=7673

actually does contain the entire patchset, not just the last part of the patch he referred to. You can just apply it all with git-am.

Cheers,

Jeremy.
Comment 11 David Disseldorp 2012-07-23 22:40:27 UTC
(In reply to comment #10)
> Karolin, despite David's message this attachment:
> 
> https://attachments.samba.org/attachment.cgi?id=7673
> 
> actually does contain the entire patchset, not just the last part of the patch
> he referred to. You can just apply it all with git-am.

Correct, all patches for v3.6-test are included with the above attachment.
Thanks a lot for the review Jeremy, and sorry for the delay in responding.

Cheers, David
Comment 12 Karolin Seeger 2012-07-24 18:54:06 UTC
Thanks for your feedback!

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

Thanks!