[GitHub] guacamole-server pull request #154: GUACAMOLE-445: Make Printer Name Configu...

[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.


---