You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/02/23 12:47:27 UTC
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-server/pull/154
GUACAMOLE-445: Make Printer Name Configurable
This adds the parameters and changes to rdpdr to make the printer name configurable in the RDP session.
I'm not sure if we had totally settled in the JIRA issue on whether or not this change was actually going to happen, but I went ahead and did it. I can close the PR if we decide against it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-server GUACAMOLE-445
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/154.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 #154
----
commit 2d8a6f97a49a84c988dc2d28b56b1954129171a4
Author: Nick Couchman <vn...@...>
Date: 2018-02-23T10:03:34Z
GUACAMLE-445: Add settings for printer name.
commit 1dd56ed01b9146605adeed4ca7ac7fdeffa8b7c2
Author: Nick Couchman <vn...@...>
Date: 2018-02-23T10:34:44Z
GUACAMOLE-445: Pass printer name from settings to RDP session.
----
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199615305
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
+ free((char *) device->device_name);
+ free(device->dos_name);
--- End diff --
Both `device->device_name` and `device->dos_name` currently point to static strings (assigned within `guac_rdpdr_register_fs()`) which should not be passed to `free()`.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625976
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
+ free((char *) device->device_name);
+ free(device->dos_name);
+ Stream_Free(device->device_announce, 1);
--- End diff --
Whoops. Yep, you're right.
Never mind.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199685199
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
Quickly added the necessary allocation and assignment for the sake of testing, and it seems to work nicely:
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170931797
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -726,6 +732,11 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_ENABLE_PRINTING, 0);
+ /* Name of redirected printer */
+ settings->printer_name =
+ guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
--- End diff --
Freed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199992472
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.h ---
@@ -35,7 +35,7 @@
* Registers a new printer device within the RDPDR plugin. This must be done
* before RDPDR connection finishes.
*/
-void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr);
+void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr, char* printer_name);
--- End diff --
Documented.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199757015
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
:man_facepalming: Doh. Yeah, that usually helps. Okay, I've initialized this and reverted my bad try below. Should be good, now.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199991453
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_service.h ---
@@ -74,23 +74,43 @@ struct guac_rdpdr_device {
int device_id;
/**
- * An arbitrary device name, used for logging purposes only.
+ * Device name, used for logging and for passthrough to the
+ * server.
*/
const char* device_name;
/**
- * Handler which will be called when the RDPDR plugin is forming the client
- * device announce list.
+ * The length of the device name, used for generating stream.
*/
- guac_rdpdr_device_announce_handler* announce_handler;
+ int device_name_len;
+
+ /**
+ * The type of RDPDR device that this represents.
+ */
+ uint32_t device_type;
+
+ /**
+ * The DOS name of the device. Max 8 bytes, including terminator.
+ */
+ char *dos_name;
--- End diff --
`const` is fine with me
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625009
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
+ free((char *) device->device_name);
+ free(device->dos_name);
+ Stream_Free(device->device_announce, 1);
--- End diff --
I think this is already present:
https://github.com/apache/guacamole-server/blob/67680bd2d51e7949453f0f7ffc7f4234a1136715/src/protocols/rdp/compat/winpr-stream.h#L68
and:
https://github.com/apache/guacamole-server/blob/67680bd2d51e7949453f0f7ffc7f4234a1136715/src/protocols/rdp/compat/winpr-stream.c#L38
And it looks like the defined `Stream_Free()` function takes two arguments??
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199945754
--- Diff: src/protocols/rdp/compat/winpr-stream.h ---
@@ -60,6 +60,7 @@
#define Stream_Buffer stream_get_head
#define Stream_Pointer stream_get_tail
#define Stream_Length stream_get_size
+#define Stream_Copy stream_copy
--- End diff --
Is this still needed as of the latest changes?
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r175278420
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
We may need to do more with respect to verifying the necessary space is actually available. Looking into the dynamic allocation of the stream buffer, we're currently allocating a static 256 bytes to contain all RDPDR devices:
https://github.com/apache/guacamole-server/blob/344ed4f42ea715a2e0e7dfd09867964ecfc320c2/src/protocols/rdp/guac_rdpdr/rdpdr_messages.c#L126
This was sufficient when the length of device names was known and this length could not possible be exceeded even if both drive and printer were enabled, but device names are now arbitrary.
Enforcing length limits on device names, truncating the names, etc. would be one possibility. Dynamically calculating the necessary space would be another (though more complex).
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r172006140
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +142,23 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
--- End diff --
Hmmm...okay. I haven't seen any issue with it behaving improperly, but I've added the null terminator explicitly to the end of the string. Doesn't feel very elegant, but it appears to work.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199991519
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -200,10 +180,43 @@ void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr) {
/* Init device */
device->rdpdr = rdpdr;
device->device_id = id;
- device->device_name = "Guacamole Printer";
+ device->device_name = printer_name;
+ device->device_name_len = guac_utf8_strlen(device->device_name);
+ device->device_type = RDPDR_DTYP_PRINT;
+ device->dos_name = "PRN1\0\0\0\0";
+
+ /* Set up device announce stream */
+ int prt_name_len = (device->device_name_len + 1) * 2;
+ device->device_announce_len = 44 + prt_name_len
+ + GUAC_PRINTER_DRIVER_LENGTH;
+ device->device_announce = Stream_New(NULL, device->device_announce_len);
+
+ /* Write common information. */
+ Stream_Write_UINT32(device->device_announce, device->device_type);
+ Stream_Write_UINT32(device->device_announce, device->device_id);
+ Stream_Write(device->device_announce, device->dos_name, 8);
+
+ /* DeviceDataLength */
+ Stream_Write_UINT32(device->device_announce, 24 + prt_name_len + GUAC_PRINTER_DRIVER_LENGTH);
+
+ /* Begin printer-specific information */
+ Stream_Write_UINT32(device->device_announce,
+ RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
+ | RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER); /* Printer flags */
+ Stream_Write_UINT32(device->device_announce, 0); /* Reserved - must be 0. */
+ Stream_Write_UINT32(device->device_announce, 0); /* PnPName Length - ignored. */
+ Stream_Write_UINT32(device->device_announce, GUAC_PRINTER_DRIVER_LENGTH);
+ Stream_Write_UINT32(device->device_announce, prt_name_len);
+ Stream_Write_UINT32(device->device_announce, 0); /* CachedFields length. */
+
+ Stream_Write(device->device_announce, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
+ guac_rdp_utf8_to_utf16((const unsigned char*) device->device_name,
+ device->device_name_len,
--- End diff --
Got it.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199946212
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.h ---
@@ -35,7 +35,7 @@
* Registers a new printer device within the RDPDR plugin. This must be done
* before RDPDR connection finishes.
*/
-void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr);
+void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr, char* printer_name);
--- End diff --
Please document the parameters of this function, now that the function is being updated.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199681339
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
I'll pull what you've got so far and see if something jumps out.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625189
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
+ free((char *) device->device_name);
+ free(device->dos_name);
--- End diff --
Ah, makes sense. My and my hang-up with static vs. dynamic allocation. I think I always equate pointers to dynamic allocation...
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199684958
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
aHA! You're missing the `Stream_New()` and corresponding assignment to `device->device_announce` for the printer device. It's there for filesystem, but not for printer:
https://github.com/apache/guacamole-server/blob/e2994778c3fed6e71f809aa589afe2db32b2e0ae/src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c#L144
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170243837
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -273,3 +273,15 @@ void guac_rdpdr_process_prn_using_xps(guac_rdpdrPlugin* rdpdr, wStream* input_st
guac_client_log(rdpdr->client, GUAC_LOG_INFO, "Printer unexpectedly switched to XPS mode");
}
+int guac_rdpdr_encode_utf16(const char* input_string, char* output_string) {
+ int output_length = (strlen(input_string)+ 1) * 2;
+ output_string = realloc(output_string, output_length);
--- End diff --
I'm not sure this is the best way to go about this - feels a little kludgy, but was trying to keep dynamic allocation.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199615001
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
--- End diff --
The `guac_rdpdrPlugin` pointed to by each `device->rdpdr` is a single, common shared instance. It is allocated externally and ultimately freed when the RDPDR plugin is unloaded:
https://github.com/apache/guacamole-server/blob/67680bd2d51e7949453f0f7ffc7f4234a1136715/src/protocols/rdp/guac_rdpdr/rdpdr_service.c#L122
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199947992
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_service.h ---
@@ -74,23 +74,43 @@ struct guac_rdpdr_device {
int device_id;
/**
- * An arbitrary device name, used for logging purposes only.
+ * Device name, used for logging and for passthrough to the
+ * server.
*/
const char* device_name;
/**
- * Handler which will be called when the RDPDR plugin is forming the client
- * device announce list.
+ * The length of the device name, used for generating stream.
*/
- guac_rdpdr_device_announce_handler* announce_handler;
+ int device_name_len;
--- End diff --
Is this useful within `guac_rdpdr_device`? Or is the length really only an internal consideration of the code generating `device_announce`?
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199992638
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -165,9 +165,6 @@ static void guac_rdpdr_device_printer_iorequest_handler(guac_rdpdr_device* devic
static void guac_rdpdr_device_printer_free_handler(guac_rdpdr_device* device) {
- free(device->rdpdr);
- free((char *) device->device_name);
--- End diff --
So, actually, the `device->device_name` is passed by reference from the `settings->printer_name` pointer, so if I try to free both `device->device_name` (in this function) and `settings->printer_name` (in the rdp_settings.c free function), I get a double free or corruption error. So I've removed it out of here.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199658855
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
Well, clearly I've messed something up - not sure if it's this or the printer naming thing, but segfaults every time...
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170899929
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -726,6 +732,11 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_ENABLE_PRINTING, 0);
+ /* Name of redirected printer */
+ settings->printer_name =
+ guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
--- End diff --
Ah, yes.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625637
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
+ free((char *) device->device_name);
+ free(device->dos_name);
--- End diff --
Removed, both here and in the printer file.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199946899
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_service.h ---
@@ -74,23 +74,43 @@ struct guac_rdpdr_device {
int device_id;
/**
- * An arbitrary device name, used for logging purposes only.
+ * Device name, used for logging and for passthrough to the
+ * server.
*/
const char* device_name;
/**
- * Handler which will be called when the RDPDR plugin is forming the client
- * device announce list.
+ * The length of the device name, used for generating stream.
*/
- guac_rdpdr_device_announce_handler* announce_handler;
+ int device_name_len;
+
+ /**
+ * The type of RDPDR device that this represents.
+ */
+ uint32_t device_type;
+
+ /**
+ * The DOS name of the device. Max 8 bytes, including terminator.
+ */
+ char *dos_name;
--- End diff --
Like `device_name`, this should probably also be `const char*`, unless you think they should both be `char*`.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199620515
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -200,10 +182,43 @@ void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr) {
/* Init device */
device->rdpdr = rdpdr;
device->device_id = id;
- device->device_name = "Guacamole Printer";
+ device->device_name = printer_name;
+ device->device_name_len = guac_utf8_strlen(device->device_name);
+ device->device_type = RDPDR_DTYP_PRINT;
+ device->dos_name = "PRN1\0\0\0\0";
+
+ /* Set up device announce stream */
+ int prt_name_len = (device->device_name_len + 1) * 2;
+ device->device_announce_len = 44 + prt_name_len
+ + GUAC_PRINTER_DRIVER_LENGTH;
+ char prt_name[prt_name_len];
--- End diff --
While C99 does allow dynamic allocation of arrays in the manner, beware that unbounded use of the stack is dangerous. I would normally recommend `malloc()` instead, but perhaps writing directly to the `wStream` buffer would be a better option in this case? There would then be no need to copy things over, leveraging `Stream_Pointer()` and `Stream_Seek()` to accomplish the same through the call to `guac_rdp_utf8_to_utf16()`.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r178643311
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
Okay, a couple of clarification questions/concerns, here:
- For tracking the size of arbitrary data within the `guac_rdpdr_device`, are you thinking all of the data - name, type, and then anything in the `data` field? Or just the size of the `data` field?
- The only potential issue with getting rid of `announce_handler`s entirely is that the complexity of the printer one is significantly larger than that of the filesystem one. I can still do this, it just means that `guac_rdpdr_send_client_device_list_announce_request()` will end up with either some `if...else if` statements or a `switch` statement to handle initialization that goes with each of the devices. Or did you envision keeping the `announce_handler`s in some form, just moving a bunch of the code over to the `guac_rdpdr_send_client_device_list_announce_request()` function?
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r171998112
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +142,23 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = strlen(device->device_name);
--- End diff --
Unfortunately, this value will not be what you expect if the device name contains any multi-byte characters. You will instead end up with the number of bytes in the string, not the number of characters. You can use `guac_utf8_strlen()`, however, which is part of libguac:
https://github.com/apache/guacamole-server/blob/bc5b01d4d8ab0c3c89a08007316d33012261f6b3/src/libguac/guacamole/unicode.h#L41-L48
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199681190
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -138,10 +138,10 @@ static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin
/* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (int i=0; i<rdpdr->devices_registered; i++) {
+ for (int i=0; i < rdpdr->devices_registered; i++) {
Stream_Write(output_stream,
- Stream_Buffer(rdpdr->devices[i].device_announce),
+ rdpdr->devices[i].device_announce,
--- End diff --
Intrepid.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199991562
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_service.h ---
@@ -74,23 +74,43 @@ struct guac_rdpdr_device {
int device_id;
/**
- * An arbitrary device name, used for logging purposes only.
+ * Device name, used for logging and for passthrough to the
+ * server.
*/
const char* device_name;
/**
- * Handler which will be called when the RDPDR plugin is forming the client
- * device announce list.
+ * The length of the device name, used for generating stream.
*/
- guac_rdpdr_device_announce_handler* announce_handler;
+ int device_name_len;
--- End diff --
Not really - best to just make it local.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199947659
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -200,10 +180,43 @@ void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr) {
/* Init device */
device->rdpdr = rdpdr;
device->device_id = id;
- device->device_name = "Guacamole Printer";
+ device->device_name = printer_name;
+ device->device_name_len = guac_utf8_strlen(device->device_name);
+ device->device_type = RDPDR_DTYP_PRINT;
+ device->dos_name = "PRN1\0\0\0\0";
+
+ /* Set up device announce stream */
+ int prt_name_len = (device->device_name_len + 1) * 2;
+ device->device_announce_len = 44 + prt_name_len
+ + GUAC_PRINTER_DRIVER_LENGTH;
+ device->device_announce = Stream_New(NULL, device->device_announce_len);
+
+ /* Write common information. */
+ Stream_Write_UINT32(device->device_announce, device->device_type);
+ Stream_Write_UINT32(device->device_announce, device->device_id);
+ Stream_Write(device->device_announce, device->dos_name, 8);
+
+ /* DeviceDataLength */
+ Stream_Write_UINT32(device->device_announce, 24 + prt_name_len + GUAC_PRINTER_DRIVER_LENGTH);
+
+ /* Begin printer-specific information */
+ Stream_Write_UINT32(device->device_announce,
+ RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
+ | RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER); /* Printer flags */
+ Stream_Write_UINT32(device->device_announce, 0); /* Reserved - must be 0. */
+ Stream_Write_UINT32(device->device_announce, 0); /* PnPName Length - ignored. */
+ Stream_Write_UINT32(device->device_announce, GUAC_PRINTER_DRIVER_LENGTH);
+ Stream_Write_UINT32(device->device_announce, prt_name_len);
+ Stream_Write_UINT32(device->device_announce, 0); /* CachedFields length. */
+
+ Stream_Write(device->device_announce, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
+ guac_rdp_utf8_to_utf16((const unsigned char*) device->device_name,
+ device->device_name_len,
--- End diff --
This should be `device_name_len + 1` to include the null terminator.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199616415
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
+ free((char *) device->device_name);
+ free(device->dos_name);
+ Stream_Free(device->device_announce, 1);
--- End diff --
Similar to `Stream_Copy()`, a compatibility macro will need to be added to allow this to build with FreeRDP 1.0, which defines `stream_free()` instead of `Stream_Free()`. Beware that `stream_free()` only accepts one parameter, always freeing the underlying buffer.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r178545222
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
Sounds good - I'll be happy to have a go at it, first, and see if I can figure out how to do it - I'll let you know if I need some help.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r171998500
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +142,23 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
--- End diff --
Beware that `guac_rdp_utf8_to_utf16()`, confusingly, does not automatically null-terminate the output string. I'm not sure at the moment whether that is an oversight, or whether things will break if that function is modified to properly null-terminate things, but this will somehow need to be taken into account.
Probably a good idea for us to update the documentation for those utility functions while we're at it, as it's problematic for the null-terminator behavior to be undocumented and different between the two.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199992432
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -200,10 +180,43 @@ void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr) {
/* Init device */
device->rdpdr = rdpdr;
device->device_id = id;
- device->device_name = "Guacamole Printer";
+ device->device_name = printer_name;
+ device->device_name_len = guac_utf8_strlen(device->device_name);
+ device->device_type = RDPDR_DTYP_PRINT;
+ device->dos_name = "PRN1\0\0\0\0";
+
+ /* Set up device announce stream */
+ int prt_name_len = (device->device_name_len + 1) * 2;
+ device->device_announce_len = 44 + prt_name_len
+ + GUAC_PRINTER_DRIVER_LENGTH;
+ device->device_announce = Stream_New(NULL, device->device_announce_len);
+
+ /* Write common information. */
+ Stream_Write_UINT32(device->device_announce, device->device_type);
+ Stream_Write_UINT32(device->device_announce, device->device_id);
+ Stream_Write(device->device_announce, device->dos_name, 8);
+
+ /* DeviceDataLength */
+ Stream_Write_UINT32(device->device_announce, 24 + prt_name_len + GUAC_PRINTER_DRIVER_LENGTH);
+
+ /* Begin printer-specific information */
+ Stream_Write_UINT32(device->device_announce,
+ RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
+ | RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER); /* Printer flags */
+ Stream_Write_UINT32(device->device_announce, 0); /* Reserved - must be 0. */
+ Stream_Write_UINT32(device->device_announce, 0); /* PnPName Length - ignored. */
+ Stream_Write_UINT32(device->device_announce, GUAC_PRINTER_DRIVER_LENGTH);
+ Stream_Write_UINT32(device->device_announce, prt_name_len);
+ Stream_Write_UINT32(device->device_announce, 0); /* CachedFields length. */
+
+ Stream_Write(device->device_announce, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
+ guac_rdp_utf8_to_utf16((const unsigned char*) device->device_name,
+ device->device_name_len,
--- End diff --
+1'd.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170243969
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +141,20 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ char* utf16_printer_name = malloc(1);
--- End diff --
As with comment above on `realloc()`, this feels a little kludgy, but allows for dynamic allocation to work.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-server/pull/154
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199992453
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_service.h ---
@@ -74,23 +74,43 @@ struct guac_rdpdr_device {
int device_id;
/**
- * An arbitrary device name, used for logging purposes only.
+ * Device name, used for logging and for passthrough to the
+ * server.
*/
const char* device_name;
/**
- * Handler which will be called when the RDPDR plugin is forming the client
- * device announce list.
+ * The length of the device name, used for generating stream.
*/
- guac_rdpdr_device_announce_handler* announce_handler;
+ int device_name_len;
+
+ /**
+ * The type of RDPDR device that this represents.
+ */
+ uint32_t device_type;
+
+ /**
+ * The DOS name of the device. Max 8 bytes, including terminator.
+ */
+ char *dos_name;
--- End diff --
const'd.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170931914
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -273,3 +273,15 @@ void guac_rdpdr_process_prn_using_xps(guac_rdpdrPlugin* rdpdr, wStream* input_st
guac_client_log(rdpdr->client, GUAC_LOG_INFO, "Printer unexpectedly switched to XPS mode");
}
+int guac_rdpdr_encode_utf16(const char* input_string, char* output_string) {
+ int output_length = (strlen(input_string)+ 1) * 2;
+ output_string = realloc(output_string, output_length);
--- End diff --
Removed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625085
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
--- End diff --
Got it, will remove.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199683587
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
...am reverting c30fd7a first, though. Writing the raw data structure is a bit too scary, especially since it only contains a pointer to the desired buffer. It shouldn't work, and I'm not sure what I'd do if it _did_ work. ;)
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625565
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_fs_service.c ---
@@ -128,6 +114,12 @@ static void guac_rdpdr_device_fs_iorequest_handler(guac_rdpdr_device* device,
}
static void guac_rdpdr_device_fs_free_handler(guac_rdpdr_device* device) {
+
+ free(device->rdpdr);
--- End diff --
Removed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625343
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -200,10 +182,43 @@ void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr) {
/* Init device */
device->rdpdr = rdpdr;
device->device_id = id;
- device->device_name = "Guacamole Printer";
+ device->device_name = printer_name;
+ device->device_name_len = guac_utf8_strlen(device->device_name);
+ device->device_type = RDPDR_DTYP_PRINT;
+ device->dos_name = "PRN1\0\0\0\0";
+
+ /* Set up device announce stream */
+ int prt_name_len = (device->device_name_len + 1) * 2;
+ device->device_announce_len = 44 + prt_name_len
+ + GUAC_PRINTER_DRIVER_LENGTH;
+ char prt_name[prt_name_len];
--- End diff --
Okay, will give it a shot.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199642034
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -165,9 +165,6 @@ static void guac_rdpdr_device_printer_iorequest_handler(guac_rdpdr_device* devic
static void guac_rdpdr_device_printer_free_handler(guac_rdpdr_device* device) {
- free(device->rdpdr);
- free((char *) device->device_name);
--- End diff --
Ah, yes. Of course.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r178485119
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
I think some of the complexity we're dealing with is due to the concept of the `announce_handler` and overlapping responsibilities between the devices and the top-level handling of RDPDR and the announce packet.
What if, instead of an `announce_handler`, we:
* Stored the device type (`RDPDR_DTYP_PRINT`, etc.) within the `guac_rdpdr_device`
* Stored the arbitrary device data and the length of that data within the `guac_rdpdr_device`
* Relied upon that within `guac_rdpdr_send_client_device_list_announce_request()`, such that it's truly only the responsibility of that function to assemble the announce packet
I think that should simplify things greatly.
If it helps, I'd be glad to take a stab at the above refactor against the current codebase, though I'm not sure how painful the rebases of these name-related PRs would be after that.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170829383
--- Diff: src/protocols/rdp/rdp_settings.c ---
@@ -726,6 +732,11 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_ENABLE_PRINTING, 0);
+ /* Name of redirected printer */
+ settings->printer_name =
+ guac_user_parse_args_string(user, GUAC_RDP_CLIENT_ARGS, argv,
--- End diff --
This will need to be freed within `guac_rdp_settings_free()`.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199681225
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
:shipit:
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170829042
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +141,20 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ char* utf16_printer_name = malloc(1);
--- End diff --
I am definitely not a fan of dynamically allocating one byte just so we can `realloc()` it immediately after. I suspect we don't actually need to do this though - see below.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r178701688
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
Well, I've taken a stab at the implementation of this, as seen in the latest commit. It feels far, far away from anywhere near good at this point, but you can take a look and let me know if I'm on the right track and make suggestions. The whole double loop thing just feels wrong, but I'm not sure how else to get the number of bytes before opening the stream and then actually write the stream data.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170828849
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -273,3 +273,15 @@ void guac_rdpdr_process_prn_using_xps(guac_rdpdrPlugin* rdpdr, wStream* input_st
guac_client_log(rdpdr->client, GUAC_LOG_INFO, "Printer unexpectedly switched to XPS mode");
}
+int guac_rdpdr_encode_utf16(const char* input_string, char* output_string) {
--- End diff --
This function as written would not actually convert things properly, as the input string is UTF-8. Characters in UTF-8 vary in byte length, and simply adding a null byte for each input byte will only work for a very small subset of UTF-8. Thankfully, we've actually done this already within `guac_rdp_utf8_to_utf16()` (defined within `unicode.h`):
https://github.com/apache/guacamole-server/blob/bc5b01d4d8ab0c3c89a08007316d33012261f6b3/src/protocols/rdp/unicode.h#L41-L57
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199991425
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.h ---
@@ -35,7 +35,7 @@
* Registers a new printer device within the RDPDR plugin. This must be done
* before RDPDR connection finishes.
*/
-void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr);
+void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr, char* printer_name);
--- End diff --
Got it.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199621521
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
I'm not sure `Stream_Copy()` is the right function/macro to use in this case. Checking the source for FreeRDP 1.1, I see `Stream_Copy()` defined as:
```c
#define Stream_Copy(_dst, _src, _n) do { \
memcpy(_dst->pointer, _src->pointer, _n); \
_dst->pointer += _n; \
_src->pointer += _n; \
} while (0)
```
It thus depends on the current write position within the buffer, and updates that write position as a result of the call.
Should we be using `Stream_Write()` and `Stream_Buffer()` instead?
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199992418
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_service.h ---
@@ -74,23 +74,43 @@ struct guac_rdpdr_device {
int device_id;
/**
- * An arbitrary device name, used for logging purposes only.
+ * Device name, used for logging and for passthrough to the
+ * server.
*/
const char* device_name;
/**
- * Handler which will be called when the RDPDR plugin is forming the client
- * device announce list.
+ * The length of the device name, used for generating stream.
*/
- guac_rdpdr_device_announce_handler* announce_handler;
+ int device_name_len;
--- End diff --
Localized.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170931887
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +141,20 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ char* utf16_printer_name = malloc(1);
--- End diff --
Removed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r172005983
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +142,23 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = strlen(device->device_name);
--- End diff --
Aha. Fixed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199625398
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -122,20 +124,28 @@ static void guac_rdpdr_send_client_capability(guac_rdpdrPlugin* rdpdr) {
static void guac_rdpdr_send_client_device_list_announce_request(guac_rdpdrPlugin* rdpdr) {
- int i;
- wStream* output_stream = Stream_New(NULL, 256);
+ /* Calculate number of bytes needed for the stream */
+ int streamBytes = 16;
+ for (int i=0; i < rdpdr->devices_registered; i++)
+ streamBytes += rdpdr->devices[i].device_announce_len;
+
+ /* Allocate the stream */
+ wStream* output_stream = Stream_New(NULL, streamBytes);
/* Write header */
Stream_Write_UINT16(output_stream, RDPDR_CTYP_CORE);
Stream_Write_UINT16(output_stream, PAKID_CORE_DEVICELIST_ANNOUNCE);
- /* List devices */
+ /* Get the stream for each of the devices. */
Stream_Write_UINT32(output_stream, rdpdr->devices_registered);
- for (i=0; i<rdpdr->devices_registered; i++) {
- guac_rdpdr_device* device = &(rdpdr->devices[i]);
- device->announce_handler(device, output_stream, i);
+ for (int i=0; i<rdpdr->devices_registered; i++) {
+
+ Stream_Copy(output_stream, rdpdr->devices[i].device_announce,
--- End diff --
I'll take a look - I might need some guidance figuring this out, but I'll give it a try.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170899796
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -273,3 +273,15 @@ void guac_rdpdr_process_prn_using_xps(guac_rdpdrPlugin* rdpdr, wStream* input_st
guac_client_log(rdpdr->client, GUAC_LOG_INFO, "Printer unexpectedly switched to XPS mode");
}
+int guac_rdpdr_encode_utf16(const char* input_string, char* output_string) {
--- End diff --
Brilliant! Will use this one, instead.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199991408
--- Diff: src/protocols/rdp/compat/winpr-stream.h ---
@@ -60,6 +60,7 @@
#define Stream_Buffer stream_get_head
#define Stream_Pointer stream_get_tail
#define Stream_Length stream_get_size
+#define Stream_Copy stream_copy
--- End diff --
Nope.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199654289
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -200,10 +182,43 @@ void guac_rdpdr_register_printer(guac_rdpdrPlugin* rdpdr) {
/* Init device */
device->rdpdr = rdpdr;
device->device_id = id;
- device->device_name = "Guacamole Printer";
+ device->device_name = printer_name;
+ device->device_name_len = guac_utf8_strlen(device->device_name);
+ device->device_type = RDPDR_DTYP_PRINT;
+ device->dos_name = "PRN1\0\0\0\0";
+
+ /* Set up device announce stream */
+ int prt_name_len = (device->device_name_len + 1) * 2;
+ device->device_announce_len = 44 + prt_name_len
+ + GUAC_PRINTER_DRIVER_LENGTH;
+ char prt_name[prt_name_len];
--- End diff --
Okay, I think I've managed to implement it the way it's supposed to be :-).
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r176919230
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
Okay, took an initial stab at a calculation...I'm sure it needs some work.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199992481
--- Diff: src/protocols/rdp/compat/winpr-stream.h ---
@@ -60,6 +60,7 @@
#define Stream_Buffer stream_get_head
#define Stream_Pointer stream_get_tail
#define Stream_Length stream_get_size
+#define Stream_Copy stream_copy
--- End diff --
Removed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170243694
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -273,3 +273,15 @@ void guac_rdpdr_process_prn_using_xps(guac_rdpdrPlugin* rdpdr, wStream* input_st
guac_client_log(rdpdr->client, GUAC_LOG_INFO, "Printer unexpectedly switched to XPS mode");
}
+int guac_rdpdr_encode_utf16(const char* input_string, char* output_string) {
--- End diff --
Not 100% certain this is the right place for this function. Also, there may be something else that already does this (swprintf?), but I had a hard time finding good examples/documentation for plain C - most everything dealt with the char16_t type which seems specific to C++.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r176918817
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
+ guac_rdp_utf8_to_utf16((const unsigned char*)device->device_name,
+ settings_length, printer_name, printer_name_length);
+ printer_name[printer_name_length - 2] = '\0';
+ printer_name[printer_name_length - 1] = '\0';
+ Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + printer_name_length);
Stream_Write_UINT32(output_stream,
RDPDR_PRINTER_ANNOUNCE_FLAG_DEFAULTPRINTER
| RDPDR_PRINTER_ANNOUNCE_FLAG_NETWORKPRINTER);
Stream_Write_UINT32(output_stream, 0); /* reserved - must be 0 */
Stream_Write_UINT32(output_stream, 0); /* PnPName length (PnPName is ultimately ignored) */
Stream_Write_UINT32(output_stream, GUAC_PRINTER_DRIVER_LENGTH); /* DriverName length */
- Stream_Write_UINT32(output_stream, GUAC_PRINTER_NAME_LENGTH); /* PrinterName length */
+ Stream_Write_UINT32(output_stream, printer_name_length); /* PrinterName length */
Stream_Write_UINT32(output_stream, 0); /* CachedFields length */
Stream_Write(output_stream, GUAC_PRINTER_DRIVER, GUAC_PRINTER_DRIVER_LENGTH);
- Stream_Write(output_stream, GUAC_PRINTER_NAME, GUAC_PRINTER_NAME_LENGTH);
+ Stream_Write(output_stream, printer_name, printer_name_length);
--- End diff --
Hmmm....okay. While enforcing limits and truncating names might be easier, I'm a little skeptical it actually will be any easier, and think probably dynamically calculating the necessary space is the way to go. I'll see what I can figure out, but I have a feeling I'll be needing some guidance on the best way to accomplish this.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r199628303
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -165,9 +165,6 @@ static void guac_rdpdr_device_printer_iorequest_handler(guac_rdpdr_device* devic
static void guac_rdpdr_device_printer_free_handler(guac_rdpdr_device* device) {
- free(device->rdpdr);
- free((char *) device->device_name);
--- End diff --
`device->device_name` *is* dynamically allocated in this case (printer) due to the changes in this PR. Only the filesystem one currently remains static and shouldn't be freed.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r175278409
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_printer.c ---
@@ -141,18 +143,25 @@ static void guac_rdpdr_device_printer_announce_handler(guac_rdpdr_device* device
Stream_Write(output_stream, "PRN1\0\0\0\0", 8); /* DOS name */
/* Printer data */
- Stream_Write_UINT32(output_stream, 24 + GUAC_PRINTER_DRIVER_LENGTH + GUAC_PRINTER_NAME_LENGTH);
+ int settings_length = guac_utf8_strlen(device->device_name);
+ int printer_name_length = (settings_length + 1) * 2;
+ char* printer_name = malloc(printer_name_length);
--- End diff --
If you will be dynamically-allocating the printer name here, it will need to be freed. Alternatively, statically-allocating the buffer may simplify things.
---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/154#discussion_r170931972
--- Diff: src/protocols/rdp/guac_rdpdr/rdpdr_messages.c ---
@@ -273,3 +273,15 @@ void guac_rdpdr_process_prn_using_xps(guac_rdpdrPlugin* rdpdr, wStream* input_st
guac_client_log(rdpdr->client, GUAC_LOG_INFO, "Printer unexpectedly switched to XPS mode");
}
+int guac_rdpdr_encode_utf16(const char* input_string, char* output_string) {
--- End diff --
Fixed.
---