From bf50dfefa7ba2aa4544a1f6037c6c38824fae548 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Thu, 10 Jul 2014 00:18:10 +0200 Subject: [PATCH 1/9] 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 Reviewed-by: Andreas Schneider (cherry picked from commit 1e83435eac2cef03fccb4cf69ef5e0bfbd710410) --- 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 136d055..2ba3b2e 100644 --- a/source3/printing/load.c +++ b/source3/printing/load.c @@ -71,5 +71,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 dd7ba62..25dd4c7 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 7e89ec4..815f89f 100644 --- a/source3/printing/printer_list.c +++ b/source3/printing/printer_list.c @@ -283,7 +283,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; NTSTATUS status; @@ -293,7 +294,11 @@ static NTSTATUS printer_list_traverse(printer_list_trv_fn_t *fn, return NT_STATUS_INTERNAL_DB_CORRUPTION; } - status = dbwrap_traverse(db, fn, private_data, NULL); + if (read_only) { + status = dbwrap_traverse_read(db, fn, private_data, NULL); + } else { + status = dbwrap_traverse(db, fn, private_data, NULL); + } return status; } @@ -363,7 +368,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; @@ -416,8 +421,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; @@ -426,7 +431,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 8e998557d496d864e77f4334ba01401508019262 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Fri, 11 Jul 2014 17:00:05 +0200 Subject: [PATCH 2/9] 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 Reviewed-by: Andreas Schneider (cherry picked from commit 4f4501ac1f35ab15f25d207c0d33e7c4d1abdf38) --- source3/printing/spoolssd.c | 34 +++++------------------------ source3/rpc_server/spoolss/srv_spoolss_nt.c | 11 +++++++++- source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 1 + source3/smbd/lanman.c | 1 + source3/smbd/server.c | 20 ----------------- 5 files changed, 17 insertions(+), 50 deletions(-) diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c index 7525fd1..c9c8a69 100644 --- a/source3/printing/spoolssd.c +++ b/source3/printing/spoolssd.c @@ -132,27 +132,6 @@ static void smb_conf_updated(struct messaging_context *msg, update_conf(ev_ctx, msg); } -static void update_pcap(struct tevent_context *ev_ctx, - struct messaging_context *msg_ctx) -{ - change_to_root_user(); - delete_and_reload_printers(ev_ctx, msg_ctx); -} - -static void 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; - - ev_ctx = talloc_get_type_abort(private_data, struct tevent_context); - - DEBUG(10, ("Got message that pcap updated. Reloading.\n")); - update_pcap(ev_ctx, msg); -} - static void spoolss_sig_term_handler(struct tevent_context *ev, struct tevent_signal *se, int signum, @@ -318,8 +297,6 @@ static bool spoolss_child_init(struct tevent_context *ev_ctx, messaging_register(msg_ctx, ev_ctx, MSG_SMB_CONF_UPDATED, smb_conf_updated); - messaging_register(msg_ctx, ev_ctx, MSG_PRINTER_PCAP, - pcap_updated); messaging_register(msg_ctx, ev_ctx, MSG_PREFORK_PARENT_EVENT, parent_ping); @@ -739,15 +716,14 @@ pid_t start_spoolssd(struct tevent_context *ev_ctx, MSG_SMB_CONF_UPDATED, smb_conf_updated); messaging_register(msg_ctx, NULL, MSG_PRINTER_UPDATE, print_queue_forward); - messaging_register(msg_ctx, ev_ctx, MSG_PRINTER_PCAP, - pcap_updated); messaging_register(msg_ctx, ev_ctx, MSG_PREFORK_CHILD_EVENT, child_ping); - /* As soon as messaging is up check if pcap has been loaded already. - * If so then we probably missed a message and should load_printers() - * ourselves. If pcap has not been loaded yet, then ignore, we will get - * a message as soon as the bq process completes the reload. */ + /* + * As soon as messaging is up check if pcap has been loaded already. + * If pcap has not been loaded yet, then ignore, as we will reload on + * client enumeration anyway. + */ if (pcap_cache_loaded()) { load_printers(ev_ctx, msg_ctx); } diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c index 64f5cbe..010d0df 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -4307,7 +4307,7 @@ 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; @@ -4319,6 +4319,15 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx, return WERR_NOMEM; } + /* + * printer shares are only updated on client enumeration. The background + * printer process updates printer_list.tdb at regular intervals. + */ + become_root(); + delete_and_reload_printers(server_event_context(), 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 011d41f..7b9ec5c 100644 --- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c +++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c @@ -484,6 +484,7 @@ static WERROR init_srv_share_info_ctr(struct pipes_struct *p, /* Ensure all the usershares are loaded. */ become_root(); + delete_and_reload_printers(server_event_context(), p->msg_ctx); load_usershare_shares(NULL, connections_snum_used); load_registry_shares(); num_services = lp_numservices(); diff --git a/source3/smbd/lanman.c b/source3/smbd/lanman.c index 0a0ab6b..d0dae36 100644 --- a/source3/smbd/lanman.c +++ b/source3/smbd/lanman.c @@ -2091,6 +2091,7 @@ static bool api_RNetShareEnum(struct smbd_server_connection *sconn, /* Ensure all the usershares are loaded. */ become_root(); + delete_and_reload_printers(sconn->ev_ctx, sconn->msg_ctx); load_registry_shares(); count = load_usershare_shares(NULL, connections_snum_used); unbecome_root(); diff --git a/source3/smbd/server.c b/source3/smbd/server.c index 918fb88..8856f43 100644 --- a/source3/smbd/server.c +++ b/source3/smbd/server.c @@ -109,24 +109,6 @@ static void smbd_parent_conf_updated(struct messaging_context *msg, } /******************************************************************* - 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(); - delete_and_reload_printers(ev_ctx, msg); -} - -/******************************************************************* Delete a statcache entry. ********************************************************************/ @@ -881,8 +863,6 @@ static bool open_sockets_smbd(struct smbd_parent_context *parent, messaging_register(msg_ctx, NULL, MSG_SMB_STAT_CACHE_DELETE, smb_stat_cache_delete); messaging_register(msg_ctx, NULL, MSG_DEBUG, smbd_msg_debug); - messaging_register(msg_ctx, ev_ctx, MSG_PRINTER_PCAP, - smb_pcap_updated); messaging_register(msg_ctx, NULL, MSG_SMB_BRL_VALIDATE, brl_revalidate); messaging_register(msg_ctx, NULL, MSG_SMB_FORCE_TDIS, -- 1.8.4.5 From e484e0c289db9db93cf0657c1d832a19d99fedff Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Tue, 22 Jul 2014 20:17:38 +0200 Subject: [PATCH 3/9] 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 Reviewed-by: Andreas Schneider (cherry picked from commit e5e6e2c796f026ee6b04f99b327941d57b9bd026) --- 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 25dd4c7..0c4bf40 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 * successfully reload */ - 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 ad61a0a..eeb193c 100644 --- a/source3/printing/print_iprint.c +++ b/source3/printing/print_iprint.c @@ -206,7 +206,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 */ @@ -342,7 +343,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: @@ -351,7 +352,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 */ @@ -359,7 +360,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")); @@ -441,14 +443,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 b00b830bb8217ea9843937d4abcef5f26d3a4f3c Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Fri, 25 Jul 2014 12:18:54 +0200 Subject: [PATCH 4/9] 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 Reviewed-by: Andreas Schneider (cherry picked from commit 6d75e20ca8acf1a55838694ac77940e21e9a1e6a) --- 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 0c4bf40..9c44584 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 355f59cd9603c70b0d7a807cf0d00170261edd90 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Wed, 23 Jul 2014 12:12:34 +0200 Subject: [PATCH 5/9] printing: return last change time with pcap_cache_loaded() Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp Reviewed-by: Andreas Schneider (cherry picked from commit 30ce835670a6aeca6fb960ea7c4fe1b982bdd5b0) --- source3/printing/load.c | 2 +- source3/printing/pcap.c | 10 ++++++++-- source3/printing/pcap.h | 2 +- source3/printing/queue_process.c | 2 +- source3/printing/spoolssd.c | 4 ++-- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/source3/printing/load.c b/source3/printing/load.c index 2ba3b2e..238998d 100644 --- a/source3/printing/load.c +++ b/source3/printing/load.c @@ -65,7 +65,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 9c44584..c5524ad 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/printing/queue_process.c b/source3/printing/queue_process.c index aa0d0fb..24d361c 100644 --- a/source3/printing/queue_process.c +++ b/source3/printing/queue_process.c @@ -390,7 +390,7 @@ void printing_subsystem_update(struct tevent_context *ev_ctx, bool force) { if (background_lpq_updater_pid != -1) { - if (pcap_cache_loaded()) { + if (pcap_cache_loaded(NULL)) { load_printers(ev_ctx, msg_ctx); } if (force) { diff --git a/source3/printing/spoolssd.c b/source3/printing/spoolssd.c index c9c8a69..59f560c 100644 --- a/source3/printing/spoolssd.c +++ b/source3/printing/spoolssd.c @@ -304,7 +304,7 @@ static bool spoolss_child_init(struct tevent_context *ev_ctx, * If so then we probably missed a message and should load_printers() * ourselves. If pcap has not been loaded yet, then ignore, we will get * a message as soon as the bq process completes the reload. */ - if (pcap_cache_loaded()) { + if (pcap_cache_loaded(NULL)) { load_printers(ev_ctx, msg_ctx); } @@ -724,7 +724,7 @@ pid_t start_spoolssd(struct tevent_context *ev_ctx, * If pcap has not been loaded yet, then ignore, as we will reload on * client enumeration anyway. */ - if (pcap_cache_loaded()) { + if (pcap_cache_loaded(NULL)) { load_printers(ev_ctx, msg_ctx); } -- 1.8.4.5 From 0892a05769af9f9a0212f11335251ab4c86114c9 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Wed, 23 Jul 2014 14:42:00 +0200 Subject: [PATCH 6/9] 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 Reviewed-by: Andreas Schneider (cherry picked from commit a2182e03a061de6c1f111ce083cb5f668fe75e4e) --- 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 1d6f9c2..6b36e68 100644 --- a/source3/smbd/server_reload.c +++ b/source3/smbd/server_reload.c @@ -31,6 +31,13 @@ #include "messages.h" #include "lib/param/loadparm.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 reload_last_pcap_time to detect this. + */ +static time_t reload_last_pcap_time = 0; + static bool snum_is_shared_printer(int snum) { return (lp_browseable(snum) && lp_snum_ok(snum) && lp_print_ok(snum)); @@ -61,6 +68,20 @@ void delete_and_reload_printers(struct tevent_context *ev, const char *pname; const char *sname; NTSTATUS status; + 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; /* Get pcap printers updated */ load_printers(ev, msg_ctx); -- 1.8.4.5 From 43519f2079d3958c66f58c4df2f099714ca9357d Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Tue, 5 Aug 2014 18:45:24 +0200 Subject: [PATCH 7/9] server: remove duplicate snum_is_shared_printer() Only keep a single definition in server_reload.c Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp Reviewed-by: Andreas Schneider (cherry picked from commit 2685df1177ffd39b1af34eb116bd7b24d4b12974) --- source3/rpc_server/spoolss/srv_spoolss_nt.c | 9 --------- source3/smbd/proto.h | 1 + source3/smbd/server_reload.c | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c index 010d0df..bb4bbe3 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -4284,15 +4284,6 @@ static WERROR construct_printer_info8(TALLOC_CTX *mem_ctx, return WERR_OK; } - -/******************************************************************** -********************************************************************/ - -static bool snum_is_shared_printer(int snum) -{ - return (lp_browseable(snum) && lp_snum_ok(snum) && lp_print_ok(snum)); -} - /******************************************************************** Spoolss_enumprinters. ********************************************************************/ diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 6153a49..327b25d 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -958,6 +958,7 @@ const struct security_token *sec_ctx_active_token(void); /* The following definitions come from smbd/server.c */ struct memcache *smbd_memcache(void); +bool snum_is_shared_printer(int snum); void delete_and_reload_printers(struct tevent_context *ev, struct messaging_context *msg_ctx); bool reload_services(struct smbd_server_connection *sconn, diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c index 6b36e68..feb8415 100644 --- a/source3/smbd/server_reload.c +++ b/source3/smbd/server_reload.c @@ -38,7 +38,7 @@ */ static time_t reload_last_pcap_time = 0; -static bool snum_is_shared_printer(int snum) +bool snum_is_shared_printer(int snum) { return (lp_browseable(snum) && lp_snum_ok(snum) && lp_print_ok(snum)); } -- 1.8.4.5 From 618950791b4567f9fb0a376d8eb55422b0365dd8 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Fri, 1 Aug 2014 16:25:59 +0200 Subject: [PATCH 8/9] smbd: split printer reload processing All printer inventory updates are currently done via delete_and_reload_printers(), which handles registry.tdb updates for added or removed printers, AD printer unpublishing on removal, as well as share service creation and deletion. This change splits this functionality into two functions such that per-client smbd processes do not perform registry.tdb updates or printer unpublishing. This is now only performed by the process that performs the printcap cache update. This change is similar to ac6604868d1325dd4c872dc0f6ab056d10ebaecf from the 3.6 branch. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10652 Signed-off-by: David Disseldorp Reviewed-by: Andreas Schneider (cherry picked from commit 2706af4d78fc9a47a4ac45b373edf276e3a9b354) --- source3/printing/queue_process.c | 100 +++++++++++++++++++++++++++++++++++++-- source3/smbd/server_reload.c | 51 +++----------------- 2 files changed, 104 insertions(+), 47 deletions(-) diff --git a/source3/printing/queue_process.c b/source3/printing/queue_process.c index 24d361c..88196b4 100644 --- a/source3/printing/queue_process.c +++ b/source3/printing/queue_process.c @@ -33,10 +33,102 @@ #include "rpc_server/rpc_config.h" #include "printing/load.h" #include "rpc_server/spoolss/srv_spoolss_nt.h" +#include "auth.h" +#include "nt_printing.h" extern pid_t start_spoolssd(struct tevent_context *ev_ctx, struct messaging_context *msg_ctx); +/** + * @brief Purge stale printers and reload from pre-populated pcap cache. + * + * This function should normally only be called as a callback on a successful + * pcap_cache_reload(). + * + * This function can cause DELETION of printers and drivers from our registry, + * so calling it on a failed pcap reload may REMOVE permanently all printers + * and drivers. + * + * @param[in] ev The event context. + * + * @param[in] msg_ctx The messaging context. + */ +static void delete_and_reload_printers_full(struct tevent_context *ev, + struct messaging_context *msg_ctx) +{ + struct auth_session_info *session_info = NULL; + struct spoolss_PrinterInfo2 *pinfo2 = NULL; + int n_services; + int pnum; + int snum; + const char *pname; + const char *sname; + NTSTATUS status; + + n_services = lp_numservices(); + pnum = lp_servicenumber(PRINTERS_NAME); + + status = make_session_info_system(talloc_tos(), &session_info); + if (!NT_STATUS_IS_OK(status)) { + DEBUG(3, ("reload_printers: " + "Could not create system session_info\n")); + /* can't remove stale printers before we + * are fully initilized */ + return; + } + + /* + * Add default config for printers added to smb.conf file and remove + * stale printers + */ + for (snum = 0; snum < n_services; snum++) { + /* avoid removing PRINTERS_NAME */ + if (snum == pnum) { + continue; + } + + /* skip no-printer services */ + if (!snum_is_shared_printer(snum)) { + continue; + } + + sname = lp_const_servicename(snum); + pname = lp_printername(session_info, snum); + + /* check printer, but avoid removing non-autoloaded printers */ + if (lp_autoloaded(snum) && !pcap_printername_ok(pname)) { + DEBUG(3, ("removing stale printer %s\n", pname)); + + if (is_printer_published(session_info, session_info, + msg_ctx, + NULL, + lp_servicename(session_info, + snum), + &pinfo2)) { + nt_printer_publish(session_info, + session_info, + msg_ctx, + pinfo2, + DSPRINT_UNPUBLISH); + TALLOC_FREE(pinfo2); + } + nt_printer_remove(session_info, session_info, msg_ctx, + pname); + } else { + DEBUG(8, ("Adding default registry entry for printer " + "[%s], if it doesn't exist.\n", sname)); + nt_printer_add(session_info, session_info, msg_ctx, + sname); + } + } + + /* finally, purge old snums */ + delete_and_reload_printers(ev, msg_ctx); + + TALLOC_FREE(session_info); +} + + /**************************************************************************** Notify smbds of new printcap data **************************************************************************/ @@ -50,7 +142,7 @@ static void reload_pcap_change_notify(struct tevent_context *ev, * This will block the process for some time (~1 sec per printer), but * it doesn't block smbd's servering clients. */ - delete_and_reload_printers(ev, msg_ctx); + delete_and_reload_printers_full(ev, msg_ctx); message_send_all(msg_ctx, MSG_PRINTER_PCAP, NULL, 0, NULL); } @@ -372,7 +464,8 @@ bool printing_subsystem_init(struct tevent_context *ev_ctx, ret = printing_subsystem_queue_tasks(ev_ctx, msg_ctx); /* Publish nt printers, this requires a working winreg pipe */ - pcap_cache_reload(ev_ctx, msg_ctx, &delete_and_reload_printers); + pcap_cache_reload(ev_ctx, msg_ctx, + &delete_and_reload_printers_full); return ret; } @@ -401,5 +494,6 @@ void printing_subsystem_update(struct tevent_context *ev_ctx, return; } - pcap_cache_reload(ev_ctx, msg_ctx, &delete_and_reload_printers); + pcap_cache_reload(ev_ctx, msg_ctx, + &delete_and_reload_printers_full); } diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c index feb8415..971c8b8 100644 --- a/source3/smbd/server_reload.c +++ b/source3/smbd/server_reload.c @@ -44,14 +44,10 @@ bool snum_is_shared_printer(int snum) } /** - * @brief Purge stale printers and reload from pre-populated pcap cache. + * @brief Purge stale printer shares and reload from pre-populated pcap cache. * * This function should normally only be called as a callback on a successful - * pcap_cache_reload() or after a MSG_PRINTER_CAP message is received. - * - * This function can cause DELETION of printers and drivers from our registry, - * so calling it on a failed pcap reload may REMOVE permanently all printers - * and drivers. + * pcap_cache_reload(), or on client enumeration. * * @param[in] ev The event context. * @@ -60,25 +56,24 @@ bool snum_is_shared_printer(int snum) void delete_and_reload_printers(struct tevent_context *ev, struct messaging_context *msg_ctx) { - struct auth_session_info *session_info = NULL; - struct spoolss_PrinterInfo2 *pinfo2 = NULL; int n_services; int pnum; int snum; const char *pname; - const char *sname; - NTSTATUS status; bool ok; time_t pcap_last_update; + TALLOC_CTX *frame = talloc_stackframe(); ok = pcap_cache_loaded(&pcap_last_update); if (!ok) { DEBUG(1, ("pcap cache not loaded\n")); + talloc_free(frame); return; } if (reload_last_pcap_time == pcap_last_update) { DEBUG(5, ("skipping printer reload, already up to date.\n")); + talloc_free(frame); return; } reload_last_pcap_time = pcap_last_update; @@ -91,15 +86,6 @@ void delete_and_reload_printers(struct tevent_context *ev, DEBUG(10, ("reloading printer services from pcap cache\n")); - status = make_session_info_system(talloc_tos(), &session_info); - if (!NT_STATUS_IS_OK(status)) { - DEBUG(3, ("reload_printers: " - "Could not create system session_info\n")); - /* can't remove stale printers before we - * are fully initilized */ - return; - } - /* * Add default config for printers added to smb.conf file and remove * stale printers @@ -115,41 +101,18 @@ void delete_and_reload_printers(struct tevent_context *ev, continue; } - sname = lp_const_servicename(snum); - pname = lp_printername(session_info, snum); + pname = lp_printername(frame, snum); /* check printer, but avoid removing non-autoloaded printers */ if (lp_autoloaded(snum) && !pcap_printername_ok(pname)) { - DEBUG(3, ("removing stale printer %s\n", pname)); - - if (is_printer_published(session_info, session_info, - msg_ctx, - NULL, - lp_servicename(session_info, - snum), - &pinfo2)) { - nt_printer_publish(session_info, - session_info, - msg_ctx, - pinfo2, - DSPRINT_UNPUBLISH); - TALLOC_FREE(pinfo2); - } - nt_printer_remove(session_info, session_info, msg_ctx, - pname); lp_killservice(snum); - } else { - DEBUG(8, ("Adding default registry entry for printer " - "[%s], if it doesn't exist.\n", sname)); - nt_printer_add(session_info, session_info, msg_ctx, - sname); } } /* Make sure deleted printers are gone */ load_printers(ev, msg_ctx); - TALLOC_FREE(session_info); + talloc_free(frame); } /**************************************************************************** -- 1.8.4.5 From 855b50d5f542394612ebc6f32433d895484250bc Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Tue, 5 Aug 2014 17:33:33 +0200 Subject: [PATCH 9/9] 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 Reviewed-by: Andreas Schneider Autobuild-User(master): Andreas Schneider Autobuild-Date(master): Fri Aug 8 16:33:50 CEST 2014 on sn-devel-104 (cherry picked from commit 1ad71f79eb473822d36d9629cf52c2fca4c53752) --- 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 bb4bbe3..335647b 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -1726,6 +1726,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(); + delete_and_reload_printers(server_event_context(), p->msg_ctx); + unbecome_root(); + /* some sanity check because you can open a printer or a print server */ /* aka: \\server\printer or \\server */ @@ -4311,7 +4321,7 @@ static WERROR enum_all_printers_info_level(TALLOC_CTX *mem_ctx, } /* - * 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