From 2d3cb6a7b2607b067e0b875fedaded7259fdb7c1 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Thu, 10 Jul 2014 00:18:10 +0200 Subject: [PATCH 1/7] printing: traverse_read the printer list for share updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The printcap update procedure involves the background printer process obtaining the printcap information from the printing backend, writing this to printer_list.tdb, and then notifying all smbd processes of the new list. The processes then all attempt to simultaneously traverse printer_list.tdb, in order to update their local share lists. With a large number of printers, and a large number of per-client smbd processes, this traversal results in significant lock contention, mostly due to the fact that the traversal is unnecessarily done with an exclusive (write) lock on the printer_list.tdb database. This commit changes the share update code path to perform a read-only traversal. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Reported-by: Alex K Reported-by: Franz Pförtsch Signed-off-by: David Disseldorp --- source3/printing/load.c | 2 +- source3/printing/pcap.c | 4 ++-- source3/printing/pcap.h | 2 +- source3/printing/printer_list.c | 17 +++++++++++------ source3/printing/printer_list.h | 4 ++-- 5 files changed, 17 insertions(+), 12 deletions(-) diff --git a/source3/printing/load.c b/source3/printing/load.c index 829c3e3..0a3de73 100644 --- a/source3/printing/load.c +++ b/source3/printing/load.c @@ -70,5 +70,5 @@ void load_printers(struct tevent_context *ev, /* load all printcap printers */ if (lp_load_printers() && lp_servicenumber(PRINTERS_NAME) >= 0) - pcap_printer_fn(lp_add_one_printer, NULL); + pcap_printer_read_fn(lp_add_one_printer, NULL); } diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c index 62db4f5..6ad8e33 100644 --- a/source3/printing/pcap.c +++ b/source3/printing/pcap.c @@ -229,11 +229,11 @@ void pcap_printer_fn_specific(const struct pcap_cache *pc, return; } -void pcap_printer_fn(void (*fn)(const char *, const char *, const char *, void *), void *pdata) +void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *pdata) { NTSTATUS status; - status = printer_list_run_fn(fn, pdata); + status = printer_list_read_run_fn(fn, pdata); if (!NT_STATUS_IS_OK(status)) { DEBUG(3, ("Failed to run fn for all printers!\n")); } diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h index 7056213..6c062c3 100644 --- a/source3/printing/pcap.h +++ b/source3/printing/pcap.h @@ -39,7 +39,7 @@ bool pcap_cache_add(const char *name, const char *comment, const char *location) bool pcap_cache_loaded(void); bool pcap_cache_replace(const struct pcap_cache *cache); void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *); -void pcap_printer_fn(void (*fn)(const char *, const char *, const char *, void *), void *); +void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *); void pcap_cache_reload(struct tevent_context *ev, struct messaging_context *msg_ctx, diff --git a/source3/printing/printer_list.c b/source3/printing/printer_list.c index 8f196a5..ef15a65 100644 --- a/source3/printing/printer_list.c +++ b/source3/printing/printer_list.c @@ -279,7 +279,8 @@ done: typedef int (printer_list_trv_fn_t)(struct db_record *, void *); static NTSTATUS printer_list_traverse(printer_list_trv_fn_t *fn, - void *private_data) + void *private_data, + bool read_only) { struct db_context *db; int ret; @@ -289,7 +290,11 @@ static NTSTATUS printer_list_traverse(printer_list_trv_fn_t *fn, return NT_STATUS_INTERNAL_DB_CORRUPTION; } - ret = db->traverse(db, fn, private_data); + if (read_only) { + ret = db->traverse_read(db, fn, private_data); + } else { + ret = db->traverse(db, fn, private_data); + } if (ret < 0) { return NT_STATUS_UNSUCCESSFUL; } @@ -356,7 +361,7 @@ NTSTATUS printer_list_clean_old(void) state.status = NT_STATUS_OK; - status = printer_list_traverse(printer_list_clean_fn, &state); + status = printer_list_traverse(printer_list_clean_fn, &state, false); if (NT_STATUS_EQUAL(status, NT_STATUS_UNSUCCESSFUL) && !NT_STATUS_IS_OK(state.status)) { status = state.status; @@ -403,8 +408,8 @@ static int printer_list_exec_fn(struct db_record *rec, void *private_data) return 0; } -NTSTATUS printer_list_run_fn(void (*fn)(const char *, const char *, const char *, void *), - void *private_data) +NTSTATUS printer_list_read_run_fn(void (*fn)(const char *, const char *, const char *, void *), + void *private_data) { struct printer_list_exec_state state; NTSTATUS status; @@ -413,7 +418,7 @@ NTSTATUS printer_list_run_fn(void (*fn)(const char *, const char *, const char * state.private_data = private_data; state.status = NT_STATUS_OK; - status = printer_list_traverse(printer_list_exec_fn, &state); + status = printer_list_traverse(printer_list_exec_fn, &state, true); if (NT_STATUS_EQUAL(status, NT_STATUS_UNSUCCESSFUL) && !NT_STATUS_IS_OK(state.status)) { status = state.status; diff --git a/source3/printing/printer_list.h b/source3/printing/printer_list.h index fb2e007..b12c192 100644 --- a/source3/printing/printer_list.h +++ b/source3/printing/printer_list.h @@ -100,6 +100,6 @@ NTSTATUS printer_list_mark_reload(void); */ NTSTATUS printer_list_clean_old(void); -NTSTATUS printer_list_run_fn(void (*fn)(const char *, const char *, const char *, void *), - void *private_data); +NTSTATUS printer_list_read_run_fn(void (*fn)(const char *, const char *, const char *, void *), + void *private_data); #endif /* _PRINTER_LIST_H_ */ -- 1.8.4.5 From 237c92dd9ca1943cf8daeb4f432863c19ac62738 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Fri, 11 Jul 2014 17:00:05 +0200 Subject: [PATCH 2/7] printing: only reload printer shares on client enum Currently, automatic printer share updates are handled in the following way: - Background printer process (BPP) forked on startup - Parent smbd and per-client children await MSG_PRINTER_PCAP messages - BPP periodically polls the printing backend for printcap data - printcap data written to printer_list.tdb - MSG_PRINTER_PCAP sent to all smbd processes following update - smbd processes all read the latest printer_list.tdb data, and update their share listings This procedure is not scalable, as all smbd processes hit printer_list.tdb in parallel, resulting in a large spike in CPU usage. This change sees smbd processes only update their printer share lists only when a client asks for this information, e.g. via NetShareEnum or EnumPrinters. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Suggested-by: Volker Lendecke Signed-off-by: David Disseldorp --- source3/printing/spoolssd.c | 17 +---------------- source3/rpc_server/spoolss/srv_spoolss_nt.c | 11 ++++++++++- source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 1 + source3/smbd/lanman.c | 3 +++ source3/smbd/server.c | 27 +++++---------------------- 5 files changed, 20 insertions(+), 39 deletions(-) diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c index 83727df..7953237 100644 --- a/source3/printing/spoolssd.c +++ b/source3/printing/spoolssd.c @@ -74,20 +74,6 @@ static void smb_conf_updated(struct messaging_context *msg, spoolss_reopen_logs(); } -static void spoolss_pcap_updated(struct messaging_context *msg, - void *private_data, - uint32_t msg_type, - struct server_id server_id, - DATA_BLOB *data) -{ - struct tevent_context *ev_ctx = talloc_get_type_abort(private_data, - struct tevent_context); - - DEBUG(10, ("Got message saying pcap was updated. Reloading.\n")); - change_to_root_user(); - reload_printers(ev_ctx, msg); -} - static void spoolss_sig_term_handler(struct tevent_context *ev, struct tevent_signal *se, int signum, @@ -206,12 +192,11 @@ void start_spoolssd(struct tevent_context *ev_ctx, exit(1); } + /* printer shares updated from printer_list.tdb on client enumeration */ messaging_register(msg_ctx, NULL, MSG_PRINTER_UPDATE, print_queue_receive); messaging_register(msg_ctx, ev_ctx, MSG_SMB_CONF_UPDATED, smb_conf_updated); - messaging_register(msg_ctx, ev_ctx, - MSG_PRINTER_PCAP, spoolss_pcap_updated); /* * Initialize spoolss with an init function to convert printers first. diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c index 8372c43..b82dd9d 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -4290,12 +4290,21 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx, uint32_t *count_p) { int snum; - int n_services = lp_numservices(); + int n_services; union spoolss_PrinterInfo *info = NULL; uint32_t count = 0; WERROR result = WERR_OK; struct dcerpc_binding_handle *b = NULL; + /* + * printer shares are only updated on client enumeration. The background + * printer process updates printer_list.tdb at regular intervals. + */ + become_root(); + reload_printers(messaging_event_context(msg_ctx), msg_ctx); + unbecome_root(); + + n_services = lp_numservices(); *count_p = 0; *info_p = NULL; diff --git a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c index b9345d6..4600da3 100644 --- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c +++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c @@ -568,6 +568,7 @@ static WERROR init_srv_share_info_ctr(struct pipes_struct *p, /* Ensure all the usershares are loaded. */ become_root(); + reload_printers(messaging_event_context(p->msg_ctx), p->msg_ctx); load_usershare_shares(); load_registry_shares(); num_services = lp_numservices(); diff --git a/source3/smbd/lanman.c b/source3/smbd/lanman.c index 0f5d6da..20e22e3 100644 --- a/source3/smbd/lanman.c +++ b/source3/smbd/lanman.c @@ -43,6 +43,7 @@ #include "passdb/machine_sid.h" #include "auth.h" #include "rpc_server/rpc_ncacn_np.h" +#include "messages.h" #ifdef CHECK_TYPES #undef CHECK_TYPES @@ -2091,6 +2092,8 @@ static bool api_RNetShareEnum(struct smbd_server_connection *sconn, /* Ensure all the usershares are loaded. */ become_root(); + reload_printers(messaging_event_context(sconn->msg_ctx), + sconn->msg_ctx); load_registry_shares(); count = load_usershare_shares(); unbecome_root(); diff --git a/source3/smbd/server.c b/source3/smbd/server.c index a26dbc4..102e8dd 100644 --- a/source3/smbd/server.c +++ b/source3/smbd/server.c @@ -111,24 +111,6 @@ static void smb_conf_updated(struct messaging_context *msg, /* printer reload triggered by background printing process */ } -/******************************************************************* - What to do when printcap is updated. - ********************************************************************/ - -static void smb_pcap_updated(struct messaging_context *msg, - void *private_data, - uint32_t msg_type, - struct server_id server_id, - DATA_BLOB *data) -{ - struct tevent_context *ev_ctx = - talloc_get_type_abort(private_data, struct tevent_context); - - DEBUG(10,("Got message saying pcap was updated. Reloading.\n")); - change_to_root_user(); - reload_printers(ev_ctx, msg); -} - static void smbd_sig_term_handler(struct tevent_context *ev, struct tevent_signal *se, int signum, @@ -1287,10 +1269,11 @@ extern void build_options(bool screen); if (is_daemon && !interactive && lp_parm_bool(-1, "smbd", "backgroundqueue", true)) { - /* background queue is responsible for printcap cache updates */ - messaging_register(smbd_server_conn->msg_ctx, - smbd_event_context(), - MSG_PRINTER_PCAP, smb_pcap_updated); + /* + * background queue is responsible for printcap cache updates. + * Other smbd processes only reload printers when a client + * issues an enumeration request. + */ start_background_queue(server_event_context(), smbd_server_conn->msg_ctx); } else { -- 1.8.4.5 From c33876d2b5b40117bfe86a75607bcd1f06871102 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Tue, 22 Jul 2014 20:17:38 +0200 Subject: [PATCH 3/7] printing: reload printer_list.tdb from in memory list This will allow in future for a single atomic printer_list.tdb update. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp --- source3/printing/pcap.c | 26 +++++++++++--------------- source3/printing/pcap.h | 8 ++++---- source3/printing/print_aix.c | 17 ++++++++++++++--- source3/printing/print_iprint.c | 16 ++++++++++------ source3/printing/print_standard.c | 8 ++++++-- source3/printing/print_svid.c | 11 +++++++---- 6 files changed, 52 insertions(+), 34 deletions(-) diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c index 6ad8e33..5173fc9 100644 --- a/source3/printing/pcap.c +++ b/source3/printing/pcap.c @@ -83,7 +83,7 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache) *pp_cache = NULL; } -bool pcap_cache_add(const char *name, const char *comment, const char *location) +static bool pcap_cache_add(const char *name, const char *comment, const char *location) { NTSTATUS status; time_t t = time_mono(NULL); @@ -132,8 +132,8 @@ void pcap_cache_reload(struct tevent_context *ev, { const char *pcap_name = lp_printcapname(); bool pcap_reloaded = False; - NTSTATUS status; bool post_cache_fill_fn_handled = false; + struct pcap_cache *pcache = NULL; DEBUG(3, ("reloading printcap cache\n")); @@ -143,12 +143,6 @@ void pcap_cache_reload(struct tevent_context *ev, return; } - status = printer_list_mark_reload(); - if (!NT_STATUS_IS_OK(status)) { - DEBUG(0, ("Failed to mark printer list for reload!\n")); - return; - } - #ifdef HAVE_CUPS if (strequal(pcap_name, "cups")) { pcap_reloaded = cups_cache_reload(ev, msg_ctx, @@ -164,26 +158,26 @@ void pcap_cache_reload(struct tevent_context *ev, #ifdef HAVE_IPRINT if (strequal(pcap_name, "iprint")) { - pcap_reloaded = iprint_cache_reload(); + pcap_reloaded = iprint_cache_reload(&pcache); goto done; } #endif #if defined(SYSV) || defined(HPUX) if (strequal(pcap_name, "lpstat")) { - pcap_reloaded = sysv_cache_reload(); + pcap_reloaded = sysv_cache_reload(&pcache); goto done; } #endif #ifdef AIX if (strstr_m(pcap_name, "/qconfig") != NULL) { - pcap_reloaded = aix_cache_reload(); + pcap_reloaded = aix_cache_reload(&pcache); goto done; } #endif - pcap_reloaded = std_pcap_cache_reload(pcap_name); + pcap_reloaded = std_pcap_cache_reload(pcap_name, &pcache); done: DEBUG(3, ("reload status: %s\n", (pcap_reloaded) ? "ok" : "error")); @@ -192,14 +186,16 @@ done: /* cleanup old entries only if the operation was successful, * otherwise keep around the old entries until we can * successfuly reaload */ - status = printer_list_clean_old(); - if (!NT_STATUS_IS_OK(status)) { - DEBUG(0, ("Failed to cleanup printer list!\n")); + + if (!pcap_cache_replace(pcache)) { + DEBUG(0, ("Failed to replace printer list!\n")); } + if (post_cache_fill_fn != NULL) { post_cache_fill_fn(ev, msg_ctx); } } + pcap_cache_destroy_specific(&pcache); return; } diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h index 6c062c3..d388d7d 100644 --- a/source3/printing/pcap.h +++ b/source3/printing/pcap.h @@ -49,7 +49,7 @@ bool pcap_printername_ok(const char *printername); /* The following definitions come from printing/print_aix.c */ -bool aix_cache_reload(void); +bool aix_cache_reload(struct pcap_cache **_pcache); /* The following definitions come from printing/print_cups.c */ @@ -60,13 +60,13 @@ bool cups_cache_reload(struct tevent_context *ev, /* The following definitions come from printing/print_iprint.c */ -bool iprint_cache_reload(void); +bool iprint_cache_reload(struct pcap_cache **_pcache); /* The following definitions come from printing/print_svid.c */ -bool sysv_cache_reload(void); +bool sysv_cache_reload(struct pcap_cache **_pcache); /* The following definitions come from printing/print_standard.c */ -bool std_pcap_cache_reload(const char *pcap_name); +bool std_pcap_cache_reload(const char *pcap_name, struct pcap_cache **_pcache); #endif /* _PRINTING_PCAP_H_ */ diff --git a/source3/printing/print_aix.c b/source3/printing/print_aix.c index 23d9a86..927a71b 100644 --- a/source3/printing/print_aix.c +++ b/source3/printing/print_aix.c @@ -29,12 +29,13 @@ #include "printing/pcap.h" #ifdef AIX -bool aix_cache_reload(void) +bool aix_cache_reload(struct pcap_cache **_pcache) { int iEtat; XFILE *pfile; char *line = NULL, *p; char *name = NULL; + struct pcap_cache *pcache = NULL; TALLOC_CTX *ctx = talloc_init("aix_cache_reload"); if (!ctx) { @@ -52,6 +53,8 @@ bool aix_cache_reload(void) iEtat = 0; /* scan qconfig file for searching : */ for (;(line = fgets_slash(NULL, 1024, pfile)); free(line)) { + bool ok; + if (*line == '*' || *line == 0) continue; @@ -67,6 +70,7 @@ bool aix_cache_reload(void) if (strcmp(p, "bsh") != 0) { name = talloc_strdup(ctx, p); if (!name) { + pcap_cache_destroy_specific(&pcache); SAFE_FREE(line); x_fclose(pfile); TALLOC_FREE(ctx); @@ -86,7 +90,10 @@ bool aix_cache_reload(void) /* name is found without stanza device */ /* probably a good printer ??? */ iEtat = 0; - if (!pcap_cache_add(name, NULL, NULL)) { + ok = pcap_cache_add_specific(&pcache, + name, NULL, NULL); + if (!ok) { + pcap_cache_destroy_specific(&pcache); SAFE_FREE(line); x_fclose(pfile); TALLOC_FREE(ctx); @@ -101,7 +108,10 @@ bool aix_cache_reload(void) } else if (strstr_m(line, "device")) { /* it's a good virtual printer */ iEtat = 0; - if (!pcap_cache_add(name, NULL, NULL)) { + ok = pcap_cache_add_specific(&pcache, + name, NULL, NULL); + if (!ok) { + pcap_cache_destroy_specific(&pcache); SAFE_FREE(line); x_fclose(pfile); TALLOC_FREE(ctx); @@ -113,6 +123,7 @@ bool aix_cache_reload(void) } } + *_pcache = pcache; x_fclose(pfile); TALLOC_FREE(ctx); return true; diff --git a/source3/printing/print_iprint.c b/source3/printing/print_iprint.c index 529f0dd..6e91747 100644 --- a/source3/printing/print_iprint.c +++ b/source3/printing/print_iprint.c @@ -204,7 +204,8 @@ static int iprint_get_server_version(http_t *http, char* serviceUri) static int iprint_cache_add_printer(http_t *http, int reqId, - char* url) + char *url, + struct pcap_cache **pcache) { ipp_t *request = NULL, /* IPP Request */ *response = NULL; /* IPP Response */ @@ -340,7 +341,7 @@ static int iprint_cache_add_printer(http_t *http, */ if (name != NULL && !secure && smb_enabled) - pcap_cache_add(name, info, NULL); + pcap_cache_add_specific(pcache, name, info, NULL); } out: @@ -349,7 +350,7 @@ static int iprint_cache_add_printer(http_t *http, return(0); } -bool iprint_cache_reload(void) +bool iprint_cache_reload(struct pcap_cache **_pcache) { http_t *http = NULL; /* HTTP connection to server */ ipp_t *request = NULL, /* IPP Request */ @@ -357,7 +358,8 @@ bool iprint_cache_reload(void) ipp_attribute_t *attr; /* Current attribute */ cups_lang_t *language = NULL; /* Default language */ int i; - bool ret = False; + bool ret = false; + struct pcap_cache *pcache = NULL; DEBUG(5, ("reloading iprint printcap cache\n")); @@ -439,14 +441,16 @@ bool iprint_cache_reload(void) char *url = ippGetString(attr, i, NULL); if (!url || !strlen(url)) continue; - iprint_cache_add_printer(http, i+2, url); + iprint_cache_add_printer(http, i+2, url, + &pcache); } } attr = ippNextAttribute(response); } } - ret = True; + ret = true; + *_pcache = pcache; out: if (response) diff --git a/source3/printing/print_standard.c b/source3/printing/print_standard.c index c4f9c5b..b5f1056 100644 --- a/source3/printing/print_standard.c +++ b/source3/printing/print_standard.c @@ -59,10 +59,11 @@ #include "printing/pcap.h" /* handle standard printcap - moved from pcap_printer_fn() */ -bool std_pcap_cache_reload(const char *pcap_name) +bool std_pcap_cache_reload(const char *pcap_name, struct pcap_cache **_pcache) { XFILE *pcap_file; char *pcap_line; + struct pcap_cache *pcache = NULL; if ((pcap_file = x_fopen(pcap_name, O_RDONLY, 0)) == NULL) { DEBUG(0, ("Unable to open printcap file %s for read!\n", pcap_name)); @@ -117,12 +118,15 @@ bool std_pcap_cache_reload(const char *pcap_name) } } - if (*name && !pcap_cache_add(name, comment, NULL)) { + if ((*name != '\0') + && !pcap_cache_add_specific(&pcache, name, comment, NULL)) { x_fclose(pcap_file); + pcap_cache_destroy_specific(&pcache); return false; } } x_fclose(pcap_file); + *_pcache = pcache; return true; } diff --git a/source3/printing/print_svid.c b/source3/printing/print_svid.c index 2226493..879661b 100644 --- a/source3/printing/print_svid.c +++ b/source3/printing/print_svid.c @@ -35,10 +35,11 @@ #include "printing/pcap.h" #if defined(SYSV) || defined(HPUX) -bool sysv_cache_reload(void) +bool sysv_cache_reload(struct pcap_cache **_pcache) { char **lines; int i; + struct pcap_cache *pcache = NULL; #if defined(HPUX) DEBUG(5, ("reloading hpux printcap cache\n")); @@ -111,14 +112,16 @@ bool sysv_cache_reload(void) *tmp = '\0'; /* add it to the cache */ - if (!pcap_cache_add(name, NULL, NULL)) { + if (!pcap_cache_add_specific(&pcache, name, NULL, NULL)) { TALLOC_FREE(lines); - return False; + pcap_cache_destroy_specific(&pcache); + return false; } } TALLOC_FREE(lines); - return True; + *_pcache = pcache; + return true; } #else -- 1.8.4.5 From 10f8bf31e9457072367df0bd3d6f183e96c74b23 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Fri, 25 Jul 2014 12:18:54 +0200 Subject: [PATCH 4/7] printing: remove pcap_cache_add() All print list updates are now done via pcap_cache_replace(), which can call into the print_list code directly. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp --- source3/printing/pcap.c | 16 ++++++---------- source3/printing/pcap.h | 1 - 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c index 5173fc9..5059f20 100644 --- a/source3/printing/pcap.c +++ b/source3/printing/pcap.c @@ -83,15 +83,6 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache) *pp_cache = NULL; } -static bool pcap_cache_add(const char *name, const char *comment, const char *location) -{ - NTSTATUS status; - time_t t = time_mono(NULL); - - status = printer_list_set_printer(talloc_tos(), name, comment, location, t); - return NT_STATUS_IS_OK(status); -} - bool pcap_cache_loaded(void) { NTSTATUS status; @@ -105,6 +96,7 @@ bool pcap_cache_replace(const struct pcap_cache *pcache) { const struct pcap_cache *p; NTSTATUS status; + time_t t = time_mono(NULL); status = printer_list_mark_reload(); if (!NT_STATUS_IS_OK(status)) { @@ -113,7 +105,11 @@ bool pcap_cache_replace(const struct pcap_cache *pcache) } for (p = pcache; p; p = p->next) { - pcap_cache_add(p->name, p->comment, p->location); + status = printer_list_set_printer(talloc_tos(), p->name, + p->comment, p->location, t); + if (!NT_STATUS_IS_OK(status)) { + return false; + } } status = printer_list_clean_old(); diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h index d388d7d..7dccf84 100644 --- a/source3/printing/pcap.h +++ b/source3/printing/pcap.h @@ -35,7 +35,6 @@ struct pcap_cache; bool pcap_cache_add_specific(struct pcap_cache **ppcache, const char *name, const char *comment, const char *location); void pcap_cache_destroy_specific(struct pcap_cache **ppcache); -bool pcap_cache_add(const char *name, const char *comment, const char *location); bool pcap_cache_loaded(void); bool pcap_cache_replace(const struct pcap_cache *cache); void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *); -- 1.8.4.5 From 60a71852d8f085578591ef86061d4850530e4755 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Wed, 23 Jul 2014 12:12:34 +0200 Subject: [PATCH 5/7] printing: return last change time with pcap_cache_loaded() Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp --- source3/printing/load.c | 2 +- source3/printing/pcap.c | 10 ++++++++-- source3/printing/pcap.h | 2 +- source3/web/swat.c | 4 ++-- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/source3/printing/load.c b/source3/printing/load.c index 0a3de73..83f1095 100644 --- a/source3/printing/load.c +++ b/source3/printing/load.c @@ -64,7 +64,7 @@ load automatic printer services from pre-populated pcap cache void load_printers(struct tevent_context *ev, struct messaging_context *msg_ctx) { - SMB_ASSERT(pcap_cache_loaded()); + SMB_ASSERT(pcap_cache_loaded(NULL)); add_auto_printers(); diff --git a/source3/printing/pcap.c b/source3/printing/pcap.c index 5059f20..027c1b2 100644 --- a/source3/printing/pcap.c +++ b/source3/printing/pcap.c @@ -83,13 +83,19 @@ void pcap_cache_destroy_specific(struct pcap_cache **pp_cache) *pp_cache = NULL; } -bool pcap_cache_loaded(void) +bool pcap_cache_loaded(time_t *_last_change) { NTSTATUS status; time_t last; status = printer_list_get_last_refresh(&last); - return NT_STATUS_IS_OK(status); + if (!NT_STATUS_IS_OK(status)) { + return false; + } + if (_last_change != NULL) { + *_last_change = last; + } + return true; } bool pcap_cache_replace(const struct pcap_cache *pcache) diff --git a/source3/printing/pcap.h b/source3/printing/pcap.h index 7dccf84..8fc9e9d 100644 --- a/source3/printing/pcap.h +++ b/source3/printing/pcap.h @@ -35,7 +35,7 @@ struct pcap_cache; bool pcap_cache_add_specific(struct pcap_cache **ppcache, const char *name, const char *comment, const char *location); void pcap_cache_destroy_specific(struct pcap_cache **ppcache); -bool pcap_cache_loaded(void); +bool pcap_cache_loaded(time_t *_last_change); bool pcap_cache_replace(const struct pcap_cache *cache); void pcap_printer_fn_specific(const struct pcap_cache *, void (*fn)(const char *, const char *, const char *, void *), void *); void pcap_printer_read_fn(void (*fn)(const char *, const char *, const char *, void *), void *); diff --git a/source3/web/swat.c b/source3/web/swat.c index f8933d2..a1a035c 100644 --- a/source3/web/swat.c +++ b/source3/web/swat.c @@ -586,7 +586,7 @@ static int save_reload(int snum) return 0; } iNumNonAutoPrintServices = lp_numservices(); - if (pcap_cache_loaded()) { + if (pcap_cache_loaded(NULL)) { load_printers(server_event_context(), server_messaging_context()); } @@ -1572,7 +1572,7 @@ const char *lang_msg_rotate(TALLOC_CTX *ctx, const char *msgid) reopen_logs(); load_interfaces(); iNumNonAutoPrintServices = lp_numservices(); - if (pcap_cache_loaded()) { + if (pcap_cache_loaded(NULL)) { load_printers(server_event_context(), server_messaging_context()); } -- 1.8.4.5 From 83f4882672eca499b0088302b5d8c17b34d23849 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Wed, 23 Jul 2014 14:42:00 +0200 Subject: [PATCH 6/7] smbd: only reprocess printer_list.tdb if it changed The per-client smbd printer share inventory is currently updated from printer_list.tdb when a client enumerates printers, via EnumPrinters or NetShareEnum. printer_list.tdb is populated by the background print process, based on the latest printcap values retrieved from the printing backend (e.g. CUPS) at regular intervals. This change ensures that per-client smbd processes don't reparse printer_list.tdb if it hasn't been updated since the last enumeration. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Suggested-by: Volker Lendecke Signed-off-by: David Disseldorp --- source3/smbd/server_reload.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c index c4c5a8d..57f7972 100644 --- a/source3/smbd/server_reload.c +++ b/source3/smbd/server_reload.c @@ -30,6 +30,13 @@ #include "auth.h" #include "messages.h" +/* + * The persistent pcap cache is populated by the background print process. Per + * client smbds should only reload their printer share inventories if this + * information has changed. Use last_reload_time to detect this. + */ +static time_t reload_last_pcap_time = 0; + /**************************************************************************** purge stale printers and reload from pre-populated pcap cache **************************************************************************/ @@ -40,6 +47,20 @@ void reload_printers(struct tevent_context *ev, int pnum; int snum; const char *pname; + bool ok; + time_t pcap_last_update; + + ok = pcap_cache_loaded(&pcap_last_update); + if (!ok) { + DEBUG(1, ("pcap cache not loaded\n")); + return; + } + + if (reload_last_pcap_time == pcap_last_update) { + DEBUG(5, ("skipping printer reload, already up to date.\n")); + return; + } + reload_last_pcap_time = pcap_last_update; n_services = lp_numservices(); pnum = lp_servicenumber(PRINTERS_NAME); -- 1.8.4.5 From 2a3f3c3a5a35078f5afc97215b1b029293c2e01f Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Wed, 6 Aug 2014 14:33:02 +0200 Subject: [PATCH 7/7] printing: reload printer shares on OpenPrinter The printer share inventory should be reloaded on open _and_ enumeration, as there are some clients, such as cupsaddsmb, that do not perform an enumeration prior to access. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp --- source3/rpc_server/spoolss/srv_spoolss_nt.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c index b82dd9d..fad992a 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -1737,6 +1737,16 @@ WERROR _spoolss_OpenPrinterEx(struct pipes_struct *p, return WERR_INVALID_PARAM; } + /* + * The printcap printer share inventory is updated on client + * enumeration. For clients that do not perform enumeration prior to + * access, such as cupssmbadd, we reinitialise the printer share + * inventory on open as well. + */ + become_root(); + reload_printers(messaging_event_context(p->msg_ctx), p->msg_ctx); + unbecome_root(); + /* some sanity check because you can open a printer or a print server */ /* aka: \\server\printer or \\server */ @@ -4297,7 +4307,7 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx, struct dcerpc_binding_handle *b = NULL; /* - * printer shares are only updated on client enumeration. The background + * printer shares are updated on client enumeration. The background * printer process updates printer_list.tdb at regular intervals. */ become_root(); -- 1.8.4.5