You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by mike-jumper <gi...@git.apache.org> on 2017/02/16 03:35:44 UTC
[GitHub] incubator-guacamole-server pull request #59: GUACAMOLE-200: Do not allow rea...
GitHub user mike-jumper opened a pull request:
https://github.com/apache/incubator-guacamole-server/pull/59
GUACAMOLE-200: Do not allow read() to block input handling
From [GUACAMOLE-200](https://issues.apache.org/jira/browse/GUACAMOLE-200):
>
> Recent changes moved the RDP printer within Guacamole from asynchronous transfer (independent of received "ack" messages) to synchronous transfer. This is fin in itself, but the `read()` call pulling more data from GhostScript in reponse to an "ack" actually occurs within the handler for that "ack". If GhostScript is busy processing things and that `read()` blocks, handling of user input as a whole gets blocked.
>
> The actual reads need to be moved into a printer-specific thread, with receipt of "ack" signalling that more data should be read, allowing the ack handler to finish regardless of GhostScript's state.
>
This change refactors the printer implementation such that printer reads occur within a separate thread.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mike-jumper/incubator-guacamole-server printer-thread
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-server/pull/59.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #59
----
commit 3fc43fba37043cb1f00bde0cccbf194c87a8dbc7
Author: Michael Jumper <mj...@apache.org>
Date: 2017-02-13T22:44:06Z
GUACAMOLE-200: Refactor RDPDR printer such that the "ack" handler cannot block.
commit d23a22b7c6e966cde6a175b38cc81f005263483d
Author: Michael Jumper <mj...@apache.org>
Date: 2017-02-14T02:42:03Z
GUACAMOLE-200: Clean up PostScript document title search logic.
commit 17093a81491d871504d614bf211efe4346fbdeff
Author: Michael Jumper <mj...@apache.org>
Date: 2017-02-14T07:08:54Z
GUACAMOLE-200: Kill any remaining print job when connection closes.
commit 1537e475af475ff4e5388c573f9f83b4ef0a13ba
Author: Michael Jumper <mj...@apache.org>
Date: 2017-02-14T07:51:33Z
GUACAMOLE-200: Refactor RDPDR print job object to top-level.
commit 5a68f932d6cc048c3ec9df8c48e64cb4e7a36202
Author: Michael Jumper <mj...@apache.org>
Date: 2017-02-14T08:02:17Z
GUACAMOLE-200: Maintain print jobs at top level. Do not rely on proper free of RDPDR plugin.
commit bf2a5885d05dca09ee4b8d73130e25e70d66a512
Author: Michael Jumper <mj...@apache.org>
Date: 2017-02-14T08:09:24Z
GUACAMOLE-200: Move print job cleanup into main RDP client thread.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #59: GUACAMOLE-200: Do not allow rea...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/59#discussion_r101442165
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -373,21 +110,25 @@ void guac_rdpdr_process_print_job_write(guac_rdpdr_device* device,
void guac_rdpdr_process_print_job_close(guac_rdpdr_device* device,
wStream* input_stream, int completion_id) {
- guac_rdpdr_printer_data* printer_data =
- (guac_rdpdr_printer_data*) device->data;
+ guac_client* client = device->rdpdr->client;
+ guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+ guac_rdp_print_job* job = (guac_rdp_print_job*) rdp_client->active_job;
+
+ /* End print job */
+ if (job != NULL) {
+ guac_rdp_print_job_free(job);
+ device->data = NULL;
--- End diff --
Erg. Definitely not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #59: GUACAMOLE-200: Do not allow rea...
Posted by jmuehlner <gi...@git.apache.org>.
Github user jmuehlner commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/59#discussion_r101441298
--- Diff: src/protocols/rdp/rdp_print_job.h ---
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RDP_PRINT_JOB_H
+#define GUAC_RDP_PRINT_JOB_H
+
+#include "config.h"
+
+#include <guacamole/client.h>
+#include <guacamole/stream.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <unistd.h>
+
+/**
+ * The maximum number of bytes in the filename of an RDP print job sent as a
+ * file over the Guacamole protocol, including NULL terminator.
+ */
+#define GUAC_RDP_PRINT_JOB_FILENAME_MAX_LENGTH 1024
+
+/**
+ * The default filename to use for the PDF output of an RDP print job if no
+ * document title can be found within the printed data.
+ */
+#define GUAC_RDP_PRINT_JOB_DEFAULT_FILENAME "guacamole-print.pdf"
+
+/**
+ * The maximum number of bytes to search through at the beginning of a
+ * PostScript document when locating the document title.
+ */
+#define GUAC_RDP_PRINT_JOB_TITLE_SEARCH_LENGTH 2048
+
+/**
+ * The current state of an RDP print job.
+ */
+typedef enum guac_rdp_print_job_state {
+
+ /**
+ * The print stream has been opened with the Guacamole client, but the
+ * client has not yet confirmed that it is ready to receive data.
+ */
+ GUAC_RDP_PRINT_JOB_WAITING_FOR_ACK,
+
+ /**
+ * The print stream has been opened with the Guacamole client, and the
+ * client has responded with an "ack", confirming that it is ready to
+ * receive data (or that data has been received and it is ready to receive
+ * more).
+ */
+ GUAC_RDP_PRINT_JOB_ACK_RECEIVED,
+
+ /**
+ * The print stream has been closed or the printer is terminating, and no
+ * further data should be sent to the client.
+ */
+ GUAC_RDP_PRINT_JOB_CLOSED
+
+} guac_rdp_print_job_state;
+
+/**
+ * Data specific to an instance of the printer device.
+ */
+typedef struct guac_rdp_print_job {
+
+ guac_client* client;
+
+ /**
+ * The user receiving the output from the print job.
+ */
+ guac_user* user;
+
+ /**
+ * The stream along which the print job output should be sent.
+ */
+ guac_stream* stream;
+
+ /**
+ * The PID of the print filter process converting PostScript data into PDF.
+ */
+ pid_t filter_pid;
+
+ /**
+ * The filename that should be used when the converted PDF output is
+ * streamed to the Guacamole user. This value will be automatically
+ * determined based on the contents of the printed document.
+ */
+ char filename[GUAC_RDP_PRINT_JOB_FILENAME_MAX_LENGTH];
+
+ /**
+ * File descriptor that should be written to when sending documents to the
+ * printer.
+ */
+ int input_fd;
+
+ /**
+ * File descriptor that should be read from when receiving output from the
+ * printer.
+ */
+ int output_fd;
+
+ /**
+ * The current state of the print stream, dependent on whether the client
+ * has acknowledged creation of the stream, whether the client has
+ * acknowledged receipt of data along the steam, and whether the print
+ * stream itself has closed.
+ */
+ guac_rdp_print_job_state state;
+
+ /**
+ * Lock which is acquired prior to modifying the state property or waiting
+ * on the state_modified conditional.
+ */
+ pthread_mutex_t state_lock;
+
+ /**
+ * Conditional which signals modification to the state property of this
+ * structure.
+ */
+ pthread_cond_t state_modified;
+
+ /**
+ * Thread which transfers data from the printer to the Guacamole client.
+ */
+ pthread_t output_thread;
+
+ /**
+ * The number of bytes received in the current print job.
+ */
+ int bytes_received;
+
+} guac_rdp_print_job;
+
+/**
+ * A blob of print data being sent to the Guacamole user.
+ */
+typedef struct guac_rdp_print_blob {
+
+ /**
+ * The print job which generated the data being sent.
+ */
+ guac_rdp_print_job* job;
+
+ /**
+ * The data being sent.
+ */
+ void* buffer;
+
+ /**
+ * The number of bytes of data being sent.
+ */
+ int length;
+
+} guac_rdp_print_blob;
+
+/**
+ * Allocates a new print job for the given user. It is expected that this
+ * function will be invoked via a call to guac_client_for_user() or
+ * guac_client_for_owner().
+ *
+ * @param user
+ * The user that should receive the output from the print job.
+ *
+ * @param data
+ * An arbitrary data parameter required by guac_client_for_user() and
+ * guac_client_for_owner() but ignored by this function. This should
+ * always be NULL.
+ *
+ * @return
+ * A pointer to a newly-allocated guac_rdp_print_job, or NULL if the
+ * print job could not be created.
+ */
+void* guac_rdp_print_job_alloc(guac_user* user, void* data);
+
+/**
+ * Writes PostScript print data to the given active print job. The print job
+ * will automatically convert this data to PDF, streaming the result to the
+ * Guacamole user associated with the print job. This function may block if
+ * the print job is not yet ready for more data.
+ *
+ * @param buffer
+ * The PostScript print data to write to the given print job.
+ *
+ * @param length
+ * The number of bytes of PostScript print data to write.
+ *
+ * @return
+ * The number of bytes written, or -1 if an error occurs which prevents
+ * further writes.
+ */
+int guac_rdp_print_job_write(guac_rdp_print_job* job,
--- End diff --
Missing javadoc for the actual job.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #59: GUACAMOLE-200: Do not allow rea...
Posted by jmuehlner <gi...@git.apache.org>.
Github user jmuehlner commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/59#discussion_r101442133
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -373,21 +110,25 @@ void guac_rdpdr_process_print_job_write(guac_rdpdr_device* device,
void guac_rdpdr_process_print_job_close(guac_rdpdr_device* device,
wStream* input_stream, int completion_id) {
- guac_rdpdr_printer_data* printer_data =
- (guac_rdpdr_printer_data*) device->data;
+ guac_client* client = device->rdpdr->client;
+ guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+ guac_rdp_print_job* job = (guac_rdp_print_job*) rdp_client->active_job;
+
+ /* End print job */
+ if (job != NULL) {
+ guac_rdp_print_job_free(job);
+ device->data = NULL;
--- End diff --
Is this what you mean to be clearing here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #59: GUACAMOLE-200: Do not allow rea...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/59#discussion_r101441534
--- Diff: src/protocols/rdp/rdp_print_job.h ---
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef GUAC_RDP_PRINT_JOB_H
+#define GUAC_RDP_PRINT_JOB_H
+
+#include "config.h"
+
+#include <guacamole/client.h>
+#include <guacamole/stream.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <unistd.h>
+
+/**
+ * The maximum number of bytes in the filename of an RDP print job sent as a
+ * file over the Guacamole protocol, including NULL terminator.
+ */
+#define GUAC_RDP_PRINT_JOB_FILENAME_MAX_LENGTH 1024
+
+/**
+ * The default filename to use for the PDF output of an RDP print job if no
+ * document title can be found within the printed data.
+ */
+#define GUAC_RDP_PRINT_JOB_DEFAULT_FILENAME "guacamole-print.pdf"
+
+/**
+ * The maximum number of bytes to search through at the beginning of a
+ * PostScript document when locating the document title.
+ */
+#define GUAC_RDP_PRINT_JOB_TITLE_SEARCH_LENGTH 2048
+
+/**
+ * The current state of an RDP print job.
+ */
+typedef enum guac_rdp_print_job_state {
+
+ /**
+ * The print stream has been opened with the Guacamole client, but the
+ * client has not yet confirmed that it is ready to receive data.
+ */
+ GUAC_RDP_PRINT_JOB_WAITING_FOR_ACK,
+
+ /**
+ * The print stream has been opened with the Guacamole client, and the
+ * client has responded with an "ack", confirming that it is ready to
+ * receive data (or that data has been received and it is ready to receive
+ * more).
+ */
+ GUAC_RDP_PRINT_JOB_ACK_RECEIVED,
+
+ /**
+ * The print stream has been closed or the printer is terminating, and no
+ * further data should be sent to the client.
+ */
+ GUAC_RDP_PRINT_JOB_CLOSED
+
+} guac_rdp_print_job_state;
+
+/**
+ * Data specific to an instance of the printer device.
+ */
+typedef struct guac_rdp_print_job {
+
+ guac_client* client;
+
+ /**
+ * The user receiving the output from the print job.
+ */
+ guac_user* user;
+
+ /**
+ * The stream along which the print job output should be sent.
+ */
+ guac_stream* stream;
+
+ /**
+ * The PID of the print filter process converting PostScript data into PDF.
+ */
+ pid_t filter_pid;
+
+ /**
+ * The filename that should be used when the converted PDF output is
+ * streamed to the Guacamole user. This value will be automatically
+ * determined based on the contents of the printed document.
+ */
+ char filename[GUAC_RDP_PRINT_JOB_FILENAME_MAX_LENGTH];
+
+ /**
+ * File descriptor that should be written to when sending documents to the
+ * printer.
+ */
+ int input_fd;
+
+ /**
+ * File descriptor that should be read from when receiving output from the
+ * printer.
+ */
+ int output_fd;
+
+ /**
+ * The current state of the print stream, dependent on whether the client
+ * has acknowledged creation of the stream, whether the client has
+ * acknowledged receipt of data along the steam, and whether the print
+ * stream itself has closed.
+ */
+ guac_rdp_print_job_state state;
+
+ /**
+ * Lock which is acquired prior to modifying the state property or waiting
+ * on the state_modified conditional.
+ */
+ pthread_mutex_t state_lock;
+
+ /**
+ * Conditional which signals modification to the state property of this
+ * structure.
+ */
+ pthread_cond_t state_modified;
+
+ /**
+ * Thread which transfers data from the printer to the Guacamole client.
+ */
+ pthread_t output_thread;
+
+ /**
+ * The number of bytes received in the current print job.
+ */
+ int bytes_received;
+
+} guac_rdp_print_job;
+
+/**
+ * A blob of print data being sent to the Guacamole user.
+ */
+typedef struct guac_rdp_print_blob {
+
+ /**
+ * The print job which generated the data being sent.
+ */
+ guac_rdp_print_job* job;
+
+ /**
+ * The data being sent.
+ */
+ void* buffer;
+
+ /**
+ * The number of bytes of data being sent.
+ */
+ int length;
+
+} guac_rdp_print_blob;
+
+/**
+ * Allocates a new print job for the given user. It is expected that this
+ * function will be invoked via a call to guac_client_for_user() or
+ * guac_client_for_owner().
+ *
+ * @param user
+ * The user that should receive the output from the print job.
+ *
+ * @param data
+ * An arbitrary data parameter required by guac_client_for_user() and
+ * guac_client_for_owner() but ignored by this function. This should
+ * always be NULL.
+ *
+ * @return
+ * A pointer to a newly-allocated guac_rdp_print_job, or NULL if the
+ * print job could not be created.
+ */
+void* guac_rdp_print_job_alloc(guac_user* user, void* data);
+
+/**
+ * Writes PostScript print data to the given active print job. The print job
+ * will automatically convert this data to PDF, streaming the result to the
+ * Guacamole user associated with the print job. This function may block if
+ * the print job is not yet ready for more data.
+ *
+ * @param buffer
+ * The PostScript print data to write to the given print job.
+ *
+ * @param length
+ * The number of bytes of PostScript print data to write.
+ *
+ * @return
+ * The number of bytes written, or -1 if an error occurs which prevents
+ * further writes.
+ */
+int guac_rdp_print_job_write(guac_rdp_print_job* job,
--- End diff --
\U0001f613
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-server pull request #59: GUACAMOLE-200: Do not allow rea...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-server/pull/59
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---