[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r12638 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r12472 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r12481 --- 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_Copystream_copy --- End diff -- Removed. ---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r12432 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r12453 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r11562 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r11519 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r11453 --- 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...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-server/pull/154#discussion_r11425 --- 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...
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...
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...
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...
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...
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_Copystream_copy --- End diff -- Is this still needed as of the latest changes? ---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
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; idevices_registered; i++) { -guac_rdpdr_device* device = &(rdpdr->devices[i]); -device->announce_handler(device, output_stream, i); +for (int i=0; idevices_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...
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; idevices_registered; i++) { -guac_rdpdr_device* device = &(rdpdr->devices[i]); -device->announce_handler(device, output_stream, i); +for (int i=0; idevices_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: ![renamed-printer](https://user-images.githubusercontent.com/4632905/42199769-2f955844-7e45-11e8-86b2-ea5ba779d591.png) ---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
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; idevices_registered; i++) { -guac_rdpdr_device* device = &(rdpdr->devices[i]); -device->announce_handler(device, output_stream, i); +for (int i=0; idevices_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...
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; idevices_registered; i++) { -guac_rdpdr_device* device = &(rdpdr->devices[i]); -device->announce_handler(device, output_stream, i); +for (int i=0; idevices_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...
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; idevices_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...
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...
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...
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...
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...
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...
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; idevices_registered; i++) { -guac_rdpdr_device* device = &(rdpdr->devices[i]); -device->announce_handler(device, output_stream, i); +for (int i=0; idevices_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...
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...
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...
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...
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...
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...
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...
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...
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; idevices_registered; i++) { -guac_rdpdr_device* device = &(rdpdr->devices[i]); -device->announce_handler(device, output_stream, i); +for (int i=0; idevices_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...
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...
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...
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...
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...
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 -- Hmmmokay. 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...
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...
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...
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...
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...
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...
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. ---
[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...
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...
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...
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...
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...
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...
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...
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 CouchmanDate: 2018-02-23T10:03:34Z GUACAMLE-445: Add settings for printer name. commit 1dd56ed01b9146605adeed4ca7ac7fdeffa8b7c2 Author: Nick Couchman Date: 2018-02-23T10:34:44Z GUACAMOLE-445: Pass printer name from settings to RDP session. ---