[PATCH] ui/cocoa: Do not rely on the first argument

[PATCH] ui/cocoa: Do not rely on the first argument

[Akihiko Odaki]

The first argument of the executable was used to get its path, but it is
not reliable because the executer can specify any arbitrary string. Use the
interfaces provided by QEMU and the platform to get those paths.

This change also changes the icon shown in the "about" window to QEMU's
cute one.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/cocoa.m | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 0ef5fdf3b7a..b3e7833e7fa 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -39,6 +39,7 @@
 #include "qapi/qapi-commands-misc.h"
 #include "sysemu/blockdev.h"
 #include "qemu-version.h"
+#include "qemu/cutils.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include <Carbon/Carbon.h>
@@ -1401,18 +1402,13 @@ - (void)make_about_window
     y = about_height - picture_height - 10;
     NSRect picture_rect = NSMakeRect(x, y, picture_width, picture_height);
 
-    /* Get the path to the QEMU binary */
-    NSString *binary_name = [NSString stringWithCString: gArgv[0]
-                                      encoding: NSASCIIStringEncoding];
-    binary_name = [binary_name lastPathComponent];
-    NSString *program_path = [[NSString alloc] initWithFormat: @"%@/%@",
-    [[NSBundle mainBundle] bundlePath], binary_name];
-
     /* Make the picture of QEMU */
     NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
                                                      picture_rect];
-    NSImage *qemu_image = [[NSWorkspace sharedWorkspace] iconForFile:
-                                                         program_path];
+    char *qemu_image_path_c = get_relocated_path(CONFIG_QEMU_ICONDIR "/hicolor/512x512/apps/qemu.png");
+    NSString *qemu_image_path = [NSString stringWithUTF8String:qemu_image_path_c];
+    g_free(qemu_image_path_c);
+    NSImage *qemu_image = [[NSImage alloc] initWithContentsOfFile:qemu_image_path];
     [picture_view setImage: qemu_image];
     [picture_view setImageScaling: NSImageScaleProportionallyUpOrDown];
     [superView addSubview: picture_view];
@@ -1427,9 +1423,7 @@ - (void)make_about_window
     [name_label setBezeled: NO];
     [name_label setDrawsBackground: NO];
     [name_label setAlignment: NSTextAlignmentCenter];
-    NSString *qemu_name = [[NSString alloc] initWithCString: gArgv[0]
-                                            encoding: NSASCIIStringEncoding];
-    qemu_name = [qemu_name lastPathComponent];
+    NSString *qemu_name = [[[NSBundle mainBundle] executablePath] lastPathComponent];
     [name_label setStringValue: qemu_name];
     [superView addSubview: name_label];
 
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] ui/cocoa: Do not rely on the first argument

[Gerd Hoffmann]

On Tue, Feb 23, 2021 at 08:06:40PM +0900, Akihiko Odaki wrote:
> The first argument of the executable was used to get its path, but it is
> not reliable because the executer can specify any arbitrary string. Use the
> interfaces provided by QEMU and the platform to get those paths.
> 
> This change also changes the icon shown in the "about" window to QEMU's
> cute one.

Looks good to me, but I'd like to have an additional macos review for
this one.

thanks,
  Gerd

Re: [PATCH] ui/cocoa: Do not rely on the first argument

[Peter Maydell]

On Tue, 23 Feb 2021 at 11:06, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> The first argument of the executable was used to get its path, but it is
> not reliable because the executer can specify any arbitrary string. Use the
> interfaces provided by QEMU and the platform to get those paths.
>
> This change also changes the icon shown in the "about" window to QEMU's
> cute one.

This patch seems to be doing two things at once:
 * we get the executable path via the OSX APIs rather than using argv[0]

 * we get an image from the icon directory rather than using the
   executable's built-in icon

Please don't put more than one change in the same patch: it makes it
harder to review.

Could you split this up so that there is one patch per change
(ie make it a 2-patch series), please ?

> @@ -1401,18 +1402,13 @@ - (void)make_about_window
>      y = about_height - picture_height - 10;
>      NSRect picture_rect = NSMakeRect(x, y, picture_width, picture_height);
>
> -    /* Get the path to the QEMU binary */
> -    NSString *binary_name = [NSString stringWithCString: gArgv[0]
> -                                      encoding: NSASCIIStringEncoding];
> -    binary_name = [binary_name lastPathComponent];
> -    NSString *program_path = [[NSString alloc] initWithFormat: @"%@/%@",
> -    [[NSBundle mainBundle] bundlePath], binary_name];
> -
>      /* Make the picture of QEMU */
>      NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
>                                                       picture_rect];
> -    NSImage *qemu_image = [[NSWorkspace sharedWorkspace] iconForFile:
> -                                                         program_path];
> +    char *qemu_image_path_c = get_relocated_path(CONFIG_QEMU_ICONDIR "/hicolor/512x512/apps/qemu.png");
> +    NSString *qemu_image_path = [NSString stringWithUTF8String:qemu_image_path_c];
> +    g_free(qemu_image_path_c);
> +    NSImage *qemu_image = [[NSImage alloc] initWithContentsOfFile:qemu_image_path];
>      [picture_view setImage: qemu_image];
>      [picture_view setImageScaling: NSImageScaleProportionallyUpOrDown];
>      [superView addSubview: picture_view];
> @@ -1427,9 +1423,7 @@ - (void)make_about_window
>      [name_label setBezeled: NO];
>      [name_label setDrawsBackground: NO];
>      [name_label setAlignment: NSTextAlignmentCenter];
> -    NSString *qemu_name = [[NSString alloc] initWithCString: gArgv[0]
> -                                            encoding: NSASCIIStringEncoding];
> -    qemu_name = [qemu_name lastPathComponent];
> +    NSString *qemu_name = [[[NSBundle mainBundle] executablePath] lastPathComponent];

The API docs for NSBundle mainBundle say it can return nil and you
always have to check the return value.

https://developer.apple.com/documentation/foundation/nsbundle/1410786-mainbundle

It's not clear to me what the fallback should be if it does return nil...

>      [name_label setStringValue: qemu_name];
>      [superView addSubview: name_label];
>
> --
> 2.24.3 (Apple Git-128)

thanks
-- PMM