Refactor geometry string parsing

Signed-off-by: Henrik Andersson <hean01@cendio.com>
Signed-off-by: Karl Mikaelsson <derfian@cendio.se>

rdesktop.c

@@ -592,6 +592,144 @@ parse_server_and_port(char *server)
 
 }
 
+// [WxH|P%|W%xH%][@DPI][+X[+Y]]|workarea
+int parse_geometry_string(const char *optarg)
+{
+	sint32 value;
+	const char *ps;
+	char *pe;
+
+	/* special keywords */
+	if (strcmp(optarg, "workarea") == 0)
+	{
+		g_sizeopt = 1;
+		return 0;
+	}
+
+	/* parse first integer */
+	ps = optarg;
+	value = strtol(ps, &pe, 10);
+	if (ps == pe || value <= 0)
+	{
+		logger(Core, Error, "invalid geometry, expected positive integer for width");
+		return -1;
+	}
+
+	g_initial_width = value;
+	ps = pe;
+
+	/* expect % or x */
+	if (*ps != '%' && *ps != 'x')
+	{
+		logger(Core, Error, "invalid geometry, expected '%%' or 'x' after width");
+		return -1;
+	}
+
+	if (*ps == '%')
+	{
+		g_sizeopt = -1;
+		ps++;
+		pe++;
+	}
+
+	if (*ps == 'x')
+	{
+		ps++;
+		value = strtol(ps, &pe, 10);
+		if (ps == pe || value <= 0)
+		{
+			logger(Core, Error, "invalid geometry, expected positive integer for height");
+			return -1;
+		}
+
+		g_initial_height = value;
+		ps = pe;
+
+		if (*ps == '%' && g_sizeopt == 0)
+		{
+			logger(Core, Error, "invalid geometry, unexpected '%%' after height");
+			return -1;
+		}
+
+		if (g_sizeopt == -1)
+		{
+			if (*ps != '%')
+			{
+				logger(Core, Error, "invalid geometry, expected '%%' after height");
+				return -1;
+			}
+			ps++;
+			pe++;
+		}
+	}
+	else
+        {
+		if (g_sizeopt == -1)
+		{
+			/* same percentage of screen for both width and height */
+			g_initial_height = g_initial_width;
+		}
+		else
+		{
+			logger(Core, Error, "invalid geometry, missing height (WxH)");
+			return -1;
+		}
+	}
+
+	/* parse optional dpi */
+	if (*ps == '@')
+	{
+		ps++;
+		pe++;
+		value = strtol(ps, &pe, 10);
+		if (ps == pe || value <= 0)
+		{
+			logger(Core, Error, "invalid geometry, expected positive integer for DPI");
+			return -1;
+		}
+
+		g_dpi = value;
+		ps = pe;
+	}
+
+	/* parse optional window position */
+	if (*ps == '+' || *ps == '-')
+	{
+		/* parse x position */
+		value = strtol(ps, &pe, 10);
+		if (ps == pe)
+		{
+			logger(Core, Error, "invalid geometry, expected an integer for X position");
+			return -1;
+		}
+
+		g_pos |= (value < 0) ? 2 : 1;
+		g_xpos = value;
+		ps = pe;
+	}
+
+	if (*ps == '+' || *ps == '-')
+	{
+		/* parse y position */
+		value = strtol(ps, &pe, 10);
+		if (ps == pe)
+		{
+			logger(Core, Error, "invalid geometry, expected an integer for Y position");
+			return -1;
+		}
+		g_pos |= (value < 0) ? 4 : 1;
+		g_ypos = value;
+		ps = pe;
+	}
+
+	if (*pe != '\0')
+	{
+		logger(Core, Error, "invalid geometry, unexpected characters at end of string");
+		return -1;
+	}
+	return 0;
+}
+
 /* Client program */
 int
 main(int argc, char *argv[])
@@ -709,67 +847,10 @@ main(int argc, char *argv[])
 			case 'g':
 				geometry_option = True;
 				g_fullscreen = False;
-				if (!strcmp(optarg, "workarea"))
+				if (parse_geometry_string(optarg) != 0)
 				{
-					g_sizeopt = 1;
-					break;
-				}
-
-				g_initial_width = strtol(optarg, &p, 10);
-				if (g_initial_width <= 0)
-				{
-					logger(Core, Error, "invalid geometry width specified");
 					return EX_USAGE;
 				}
-
-				if (*p == 'x')
-					g_initial_height = strtol(p + 1, &p, 10);
-
-				if (g_initial_height <= 0)
-				{
-					logger(Core, Error, "invalid geometry height specified");
-					return EX_USAGE;
-				}
-
-				if (*p == '%')
-				{
-					g_sizeopt = -g_initial_width;
-					g_initial_width = g_sizeopt;
-
-					if (*(p + 1) == 'x')
-					{
-						g_initial_height = -strtol(p + 2, &p, 10);
-					}
-					else
-					{
-						g_initial_height = g_sizeopt;
-					}
-
-					p++;
-				}
-
-				if (*p == '@')
-				{
-					g_dpi = strtol(p + 1, &p, 10);
-					if (g_dpi <= 0)
-					{
-						logger(Core, Error, "invalid DPI: expected a positive integer after @\n");
-						return EX_USAGE;
-					}
-				}
-
-				if (*p == '+' || *p == '-')
-				{
-					g_pos |= (*p == '-') ? 2 : 1;
-					g_xpos = strtol(p, &p, 10);
-
-				}
-				if (*p == '+' || *p == '-')
-				{
-					g_pos |= (*p == '-') ? 4 : 1;
-					g_ypos = strtol(p, NULL, 10);
-				}
-
 				break;
 
 			case 'f':

tests/Makefile

@@ -2,7 +2,7 @@ CC=gcc
 CFLAGS=-fPIC -Wall -Wextra -ggdb -gdwarf-2 -g3
 CGREEN_RUNNER=cgreen-runner
 
-TESTS=resize rdp xwin utils
+TESTS=resize rdp xwin utils parse_geometry
 
 
 RDP_MOCKS=ui_mock.o bitmap_mock.o secure_mock.o ssl_mock.o mppc_mock.o \
@@ -19,6 +19,9 @@ RESIZE_MOCKS=x11_mock.o cache_mock.o xclip_mock.o xkeymap_mock.o seamless_mock.o
 	ssl_mock.o mppc_mock.o pstcache_mock.o orders_mock.o rdesktop_mock.o rdp5_mock.o \
 	tcp_mock.o licence_mock.o mcs_mock.o channels_mock.o
 
+PARSE_MOCKS=ui_mock.o rdpdr_mock.o rdpedisp_mock.o ssl_mock.o ctrl_mock.o secure_mock.o \
+	tcp_mock.o dvc_mock.o rdp_mock.o cache_mock.o cliprdr_mock.o disk_mock.o lspci_mock.o \
+	parallel_mock.o printer_mock.o serial_mock.o xkeymap_mock.o utils_mock.o
 
 all: test
 
@@ -43,6 +46,8 @@ utils: utils_test.o $(UTILS_MOCKS)
 resize: resize_test.o $(RESIZE_MOCKS)
 	$(CC) $(CFLAGS) -shared -lcgreen -o $@ $^ -lX11 -lXcursor
 
+parse_geometry: parse_geometry_test.o $(PARSE_MOCKS) ../rdesktop.c
+	$(CC) $(CFLAGS) -shared -lcgreen -o $@ parse_geometry_test.o $(PARSE_MOCKS)
 
 .PHONY: clean
 clean:

tests/cache_mock.c

@@ -44,3 +44,9 @@ cache_put_desktop(uint32 offset, int cx, int cy, int scanline,
 {
   mock(offset, cx, cy, scanline, bytes_per_pixel, data);
 }
+
+void
+cache_save_state()
+{
+  mock();
+}

tests/cliprdr_mock.c

@@ -0,0 +1,8 @@
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+
+void
+cliprdr_set_mode(const char *optarg)
+{
+  mock(optarg);
+}

tests/ctrl_mock.c

@@ -12,3 +12,21 @@ ctrl_check_fds(fd_set * rfds, fd_set * wfds)
 {
   mock(rfds, wfds);
 }
+
+int
+ctrl_init(const char *user, const char *domain, const char *host)
+{
+  return mock(user, domain, host);
+}
+
+RD_BOOL
+ctrl_is_slave()
+{
+  return mock();
+}
+
+int
+ctrl_send_command(const char *cmd, const char *args)
+{
+  return mock(cmd, args);
+}

tests/disk_mock.c

@@ -0,0 +1,8 @@
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+
+int
+disk_enum_devices(uint32 *id, char *optarg)
+{
+  return mock(id, optarg);
+}

tests/lspci_mock.c

@@ -0,0 +1,9 @@
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+
+
+RD_BOOL
+lspci_init(void)
+{
+  return mock();
+}

tests/parallel_mock.c

@@ -0,0 +1,8 @@
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+
+int
+parallel_enum_devices(uint32 * id, char *optarg)
+{
+  return mock(id, optarg);
+}

tests/parse_geometry_test.c

@@ -0,0 +1,228 @@
+#include <cgreen/cgreen.h>
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+#include "../proto.h"
+#include <locale.h>
+#include <langinfo.h>
+
+#define always_expect_error_log() always_expect(logger, when(lvl, is_equal_to(Error)))
+
+/* Boilerplate */
+Describe(ParseGeometry);
+BeforeEach(ParseGeometry) {};
+AfterEach(ParseGeometry) {};
+
+/* Global Variables.. :( */
+int g_tcp_port_rdp;
+RDPDR_DEVICE g_rdpdr_device[16];
+uint32 g_num_devices;
+char *g_rdpdr_clientname;
+RD_BOOL g_using_full_workarea;
+
+#define PACKAGE_VERSION "test"
+
+#include "../rdesktop.c"
+
+
+Ensure(ParseGeometry, HandlesWxH)
+{
+  g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345"), is_equal_to(0));
+
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, FailsOnMissingHeight)
+{
+  always_expect_error_log();
+
+  g_initial_width = g_initial_height = g_sizeopt = 0;
+  assert_that(parse_geometry_string("1234"), is_equal_to(-1));
+
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(0));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, FailsOnMissingHeightVariant2)
+{
+  always_expect_error_log();
+
+  g_initial_width = g_initial_height = g_sizeopt = 0;
+  assert_that(parse_geometry_string("1234x"), is_equal_to(-1));
+
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(0));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, HandlesPercentageOfScreen)
+{
+  g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("80%"), is_equal_to(0));
+
+  assert_that(g_initial_width, is_equal_to(80));
+  assert_that(g_initial_height, is_equal_to(80));
+  assert_that(g_sizeopt, is_equal_to(-1));
+}
+
+Ensure(ParseGeometry, HandlesSpecificWidthAndHeightPercentageOfScreen)
+{
+  g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("100%x60%"), is_equal_to(0));
+
+  assert_that(g_initial_width, is_equal_to(100));
+  assert_that(g_initial_height, is_equal_to(60));
+  assert_that(g_sizeopt, is_equal_to(-1));
+}
+
+Ensure(ParseGeometry, HandlesSpecifiedDPI)
+{
+  g_dpi = g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345@234"), is_equal_to(0));
+
+  assert_that(g_dpi, is_equal_to(234));
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+
+Ensure(ParseGeometry, HandlesSpecifiedXPosition)
+{
+  g_xpos = g_ypos = g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345+123"), is_equal_to(0));
+
+  assert_that(g_xpos, is_equal_to(123));
+  assert_that(g_ypos, is_equal_to(0));
+  assert_that(g_pos, is_equal_to(1));
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, HandlesSpecifiedNegativeXPosition)
+{
+  g_ypos = g_xpos = g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345-500"), is_equal_to(0));
+
+  assert_that(g_xpos, is_equal_to(-500));
+  assert_that(g_ypos, is_equal_to(0));
+  assert_that(g_pos, is_equal_to(2));
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, HandlesSpecifiedNegativeXAndYPosition)
+{
+  g_ypos = g_xpos = g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345-500-501"), is_equal_to(0));
+
+  assert_that(g_xpos, is_equal_to(-500));
+  assert_that(g_ypos, is_equal_to(-501));
+  assert_that(g_pos, is_equal_to(2 | 4));
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, HandlesSpecifiedXandYPosition)
+{
+  g_xpos = g_ypos = g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345+123+234"), is_equal_to(0));
+
+  assert_that(g_xpos, is_equal_to(123));
+  assert_that(g_ypos, is_equal_to(234));
+  assert_that(g_pos, is_equal_to(1));
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, HandlesSpecifiedXandYPositionWithDPI)
+{
+  g_dpi = g_xpos = g_ypos = g_initial_width = g_initial_height = g_sizeopt = 0;
+
+  assert_that(parse_geometry_string("1234x2345@678+123+234"), is_equal_to(0));
+
+  assert_that(g_dpi, is_equal_to(678));
+  assert_that(g_xpos, is_equal_to(123));
+  assert_that(g_ypos, is_equal_to(234));
+  assert_that(g_initial_width, is_equal_to(1234));
+  assert_that(g_initial_height, is_equal_to(2345));
+  assert_that(g_sizeopt, is_equal_to(0));
+}
+
+Ensure(ParseGeometry, HandlesSpecialNameWorkarea)
+{
+  assert_that(parse_geometry_string("workarea"), is_equal_to(0));
+
+  assert_that(g_sizeopt, is_equal_to(1));
+}
+
+
+Ensure(ParseGeometry, FailsOnNegativeDPI)
+{
+  always_expect_error_log();
+
+  assert_that(parse_geometry_string("1234x2345@-105"), is_equal_to(-1));
+}
+
+
+Ensure(ParseGeometry, FailsOnNegativeWidth)
+{
+  always_expect_error_log();
+
+  assert_that(parse_geometry_string("-1234x2345"), is_equal_to(-1));
+}
+
+
+Ensure(ParseGeometry, FailsOnNegativeHeight)
+{
+  always_expect_error_log();
+
+  assert_that(parse_geometry_string("1234x-2345"), is_equal_to(-1));
+}
+
+Ensure(ParseGeometry, FailsOnMixingPixelsAndPercents)
+{
+  always_expect_error_log();
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1234%x2345"), is_equal_to(-1));
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1234x2345%"), is_equal_to(-1));
+}
+
+Ensure(ParseGeometry, FailsOnGarbageAtEndOfString)
+{
+  always_expect_error_log();
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1234%1239123081232345abcdefgadkfjafa4af048"), is_equal_to(-1));
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1235abcer9823461"), is_equal_to(-1));
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1235%x123%+123123+123123asdkjfasdf"), is_equal_to(-1));
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1235%x123%@123asdkjfasdf"), is_equal_to(-1));
+
+  g_sizeopt = 0;
+  assert_that(parse_geometry_string("1235%x123%@123+1-2asdkjfasdf"), is_equal_to(-1));
+}
+

tests/printer_mock.c

@@ -0,0 +1,8 @@
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+
+int
+printer_enum_devices(uint32 * id, char *optarg)
+{
+  return mock(id, optarg);
+}

tests/rdp_mock.c

@@ -13,3 +13,27 @@ rdp_send_suppress_output_pdu(enum RDP_SUPPRESS_STATUS allowupdates)
   mock(allowupdates);
 }
 
+RD_BOOL
+rdp_connect(char *server, uint32 flags, char *domain, char *password, char *command,
+	    char *directory, RD_BOOL reconnect)
+{
+  return mock(server, flags, domain, password, command, directory, reconnect);
+}
+
+void
+rdp_disconnect()
+{
+  mock();
+}
+
+void
+rdp_main_loop(RD_BOOL * deactivated, uint32 * ext_disc_reason)
+{
+  mock(deactivated, ext_disc_reason);
+}
+
+void
+rdp_reset_state()
+{
+  mock();
+}

tests/rdpdr_mock.c

@@ -12,3 +12,9 @@ rdpdr_check_fds(fd_set * rfds, fd_set * wfds, RD_BOOL timed_out)
 {
   mock(rfds, wfds, timed_out);
 }
+
+RD_BOOL
+rdpdr_init()
+{
+  return mock();
+}

tests/rdpedisp_mock.c

@@ -1,6 +1,12 @@
 #include <cgreen/mocks.h>
 #include "../rdesktop.h"
 
+void
+rdpedisp_init()
+{
+  mock();
+}
+
 RD_BOOL
 rdpedisp_is_available()
 {

tests/secure_mock.c

@@ -30,3 +30,15 @@ void sec_send(STREAM s, uint32 flags)
 {
   mock(s, flags);
 }
+
+void
+sec_hash_sha1_16(uint8 * out, uint8 * in, uint8 * salt1)
+{
+  mock(out, in, salt1);
+}
+
+void
+sec_hash_to_string(char *out, int out_size, uint8 * in, int in_size)
+{
+  mock(out, out_size, in, in_size);
+}

tests/serial_mock.c

@@ -0,0 +1,8 @@
+#include <cgreen/mocks.h>
+#include "../rdesktop.h"
+
+int
+serial_enum_devices(uint32 * id, char *optarg)
+{
+  return mock(id, optarg);
+}

tests/tcp_mock.c

@@ -5,3 +5,9 @@ char *tcp_get_address()
 {
   return (char *) mock();
 }
+
+void
+tcp_run_ui(RD_BOOL run)
+{
+  mock(run);
+}

tests/ui_mock.c

@@ -70,3 +70,51 @@ void ui_set_clip(int x,int y, int cx, int cy)
 {
   mock(x,y,cx,cy);
 }
+
+RD_BOOL
+ui_create_window(uint32 width, uint32 height)
+{
+  return mock(width, height);
+}
+
+void
+ui_deinit()
+{
+  mock();
+}
+
+void
+ui_destroy_window()
+{
+  mock();
+}
+
+void
+ui_init_connection()
+{
+  mock();
+}
+
+RD_BOOL
+ui_have_window()
+{
+  return mock();
+}
+
+RD_BOOL
+ui_init()
+{
+  return mock();
+}
+
+void
+ui_reset_clip()
+{
+  mock();
+}
+
+void
+ui_seamless_end()
+{
+  mock();
+}

tests/utils_mock.c

@@ -0,0 +1,19 @@
+#include <cgreen/cgreen.h>
+#include <cgreen/mocks.h>
+
+#include "../rdesktop.h"
+
+uint32 utils_djb2_hash(const char *str) { return mock(str); }
+char *utils_string_escape(const char *str) { return (char *)mock(str); }
+char *utils_string_unescape(const char *str) { return (char *)mock(str); }
+int utils_locale_to_utf8(const char *src, size_t is, char *dest, size_t os) { return mock(src, is, dest, os); }
+int utils_mkdir_safe(const char *path, int mask) { return mock(path, mask); }
+int utils_mkdir_p(const char *path, int mask) { return mock(path, mask); }
+void utils_calculate_dpi_scale_factors(uint32 width, uint32 height, uint32 dpi,
+				       uint32 *physwidth, uint32 *physheight,
+				       uint32 *desktopscale, uint32 *devicescale) { mock(width, height, dpi, physwidth, physheight, desktopscale, devicescale); }
+void utils_apply_session_size_limitations(uint32 *width, uint32 *height) { mock(width, height); }
+
+void logger(log_subject_t c, log_level_t lvl, char *format, ...) { mock(c, lvl, format); }
+void logger_set_verbose(int verbose) { mock(verbose); }
+void logger_set_subjects(char *subjects) { mock(subjects); }

tests/xkeymap_mock.c

@@ -61,3 +61,9 @@ uint16 ui_get_numlock_state(unsigned int state)
 {
   return mock(state);
 }
+
+RD_BOOL
+xkeymap_from_locale(const char *locale)
+{
+  return mock(locale);
+}

xwin.c

@@ -1983,14 +1983,9 @@ ui_init_connection(void)
 		if (-g_sizeopt >= 100)
 			g_using_full_workarea = True;
 
-		if (g_initial_width > 0)
-			g_initial_width = g_sizeopt;
-
-		if (g_initial_height > 0)
-			g_initial_height = g_sizeopt;
-
-		g_initial_height = HeightOfScreen(g_screen) * (-g_initial_height) / 100;
-		g_initial_width = WidthOfScreen(g_screen) * (-g_initial_width) / 100;
+		/* g_initial_width/height holds percentage of screen in each axis */
+		g_initial_height = HeightOfScreen(g_screen) * g_initial_height / 100;
+		g_initial_width = WidthOfScreen(g_screen) * g_initial_width / 100;
 	}
 	else if (g_sizeopt == 1)
 	{