Re: [Qemu-devel] [PATCH v2] ui/cocoa.m: Add Mount image file menu item

[Markus Armbruster]

Only reviewing around qmp_device_add() and such, ignoring the GUI part.

Programmingkid  writes:

> Add "Mount Image File..." and a "Eject Image File" menu items to
> cocoa interface. This patch makes sharing files between the
> host and the guest user-friendly.
>
> The "Mount Image File..." menu item displays a dialog box having the
> user pick an image file to use in QEMU. The image file is setup as
> a USB flash drive. The user can do the equivalent of removing the
> flash drive by selecting the file in the "Eject Image File" submenu.
>
> Signed-off-by: John Arbuckle 
>
> ---
> Detects if the user actually specified the -usb option.
> Removed the sendMonitorCommand() function. 
> Replaced a lot of code with C interface equivalent of monitor commands
> drive_add and device_add. 
>
>  ui/cocoa.m |  228 
> +++-
>  1 files changed, 227 insertions(+), 1 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 334e6f6..df1faea 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -31,6 +31,8 @@
>  #include "sysemu/sysemu.h"
>  #include "qmp-commands.h"
>  #include "sysemu/blockdev.h"
> +#include "include/monitor/qdev.h"
> +#include "hw/boards.h"
>  
>  #ifndef MAC_OS_X_VERSION_10_5
>  #define MAC_OS_X_VERSION_10_5 1050
> @@ -52,6 +54,8 @@
>  #endif
>  
>  #define cgrect(nsrect) (*(CGRect *)&(nsrect))
> +#define USB_DISK_ID "USB_DISK"
> +#define EJECT_IMAGE_FILE_TAG 2099
>  
>  typedef struct {
>  int width;
> @@ -829,6 +833,9 @@ QemuCocoaView *cocoaView;
>  - (void)powerDownQEMU:(id)sender;
>  - (void)ejectDeviceMedia:(id)sender;
>  - (void)changeDeviceMedia:(id)sender;
> +- (void)mountImageFile:(id)sender;
> +- (void)ejectImageFile:(id)sender;
> +- (void)updateEjectImageMenuItems;
>  @end
>  
>  @implementation QemuCocoaAppController
> @@ -1125,6 +1132,179 @@ QemuCocoaView *cocoaView;
>  }
>  }
>  
> +/* Displays a dialog box asking the user for an image file to mount */
> +- (void)mountImageFile:(id)sender
> +{
> +/* Display the file open dialog */
> +NSOpenPanel * openPanel;
> +openPanel = [NSOpenPanel openPanel];
> +[openPanel setCanChooseFiles: YES];
> +[openPanel setAllowsMultipleSelection: NO];
> +[openPanel setAllowedFileTypes: supportedImageFileTypes];
> +if([openPanel runModal] == NSFileHandlingPanelOKButton) {
> +NSString * file = [[[openPanel URLs] objectAtIndex: 0] path];
> +if(file == nil) {
> +NSBeep();
> +QEMU_Alert(@"Failed to convert URL to file path!");
> +return;
> +}
> +
> +static int usbDiskCount;
> +char *idString, *fileName, *driveOptions, *fileNameHint;
> +
> +/* Create the file name hint */
> +NSString *stringBuffer;
> +const int fileNameHintSize = 10;
> +stringBuffer = [file lastPathComponent];
> +stringBuffer = [stringBuffer stringByDeletingPathExtension];
> +if([stringBuffer length] > fileNameHintSize) {
> +stringBuffer = [stringBuffer substringToIndex: fileNameHintSize];
> +}
> +fileNameHint = g_strdup_printf("%s",
> +[stringBuffer cStringUsingEncoding: 
> NSASCIIStringEncoding]);
> +
> +idString = g_strdup_printf("%s_%s_%d", USB_DISK_ID, fileNameHint,
> +
> usbDiskCount++);

Indentation is off, long line.

We don't use CamelCase for variable names in QEMU.  See CODING_STYLE,
section 3. Naming.

More of the same elsewhere.

Your system-generated ID can theoretically clash with a user-specified
ID.  We should reserve a name space for system-generated IDs.  Related:
our discussion Re: Should we auto-generate IDs?  Anyway, wider issue,
not something this patch can solve.

> +
> +fileName = g_strdup_printf("%s",
> +[file cStringUsingEncoding: 
> NSASCIIStringEncoding]);
> +
> +/* drive_add equivalent code */
> +driveOptions = g_strdup_printf("if=none,id=%s,file=%s", idString,
> + 
> fileName);
> +DriveInfo *dinfo;
> +QemuOpts *opts;
> +MachineClass *mc;

No declarations in the middle of blocks, please.  See CODING_STYLE
section 5. Declarations.

More of the same elsewhere.

> +
> +opts = drive_def(driveOptions);

Breaks when fileName contains a comma.  Fine example of why formatting
stuff only to parse it apart again is a bad idea.  Use drive_add()
instead.  Roughly like

opts = drive_add(IF_NONE, -1, file_name, "");

You could've found this yourself easily had you slowed down a bit to
examine how we create drive QemuOpts elsewhere.

> +if (!opts) {
> +QEMU_Alert(@"Error: could not create QemuOpts object!");
> +return;
> +}
> +
> +mc = MACHINE_GET_CLASS(current_machine);
> +dinfo = 

Re: [Qemu-devel] [PATCH v2] ui/cocoa.m: Add Mount image file menu item

[Programmingkid]

On Sep 11, 2015, at 3:40 AM, Markus Armbruster wrote:

> Only reviewing around qmp_device_add() and such, ignoring the GUI part.
> 
> Programmingkid  writes:
> 
>> Add "Mount Image File..." and a "Eject Image File" menu items to
>> cocoa interface. This patch makes sharing files between the
>> host and the guest user-friendly.
>> 
>> The "Mount Image File..." menu item displays a dialog box having the
>> user pick an image file to use in QEMU. The image file is setup as
>> a USB flash drive. The user can do the equivalent of removing the
>> flash drive by selecting the file in the "Eject Image File" submenu.
>> 
>> Signed-off-by: John Arbuckle 
>> 
>> ---
>> Detects if the user actually specified the -usb option.
>> Removed the sendMonitorCommand() function. 
>> Replaced a lot of code with C interface equivalent of monitor commands
>> drive_add and device_add. 
>> 
>> ui/cocoa.m |  228 
>> +++-
>> 1 files changed, 227 insertions(+), 1 deletions(-)
>> 
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 334e6f6..df1faea 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -31,6 +31,8 @@
>> #include "sysemu/sysemu.h"
>> #include "qmp-commands.h"
>> #include "sysemu/blockdev.h"
>> +#include "include/monitor/qdev.h"
>> +#include "hw/boards.h"
>> 
>> #ifndef MAC_OS_X_VERSION_10_5
>> #define MAC_OS_X_VERSION_10_5 1050
>> @@ -52,6 +54,8 @@
>> #endif
>> 
>> #define cgrect(nsrect) (*(CGRect *)&(nsrect))
>> +#define USB_DISK_ID "USB_DISK"
>> +#define EJECT_IMAGE_FILE_TAG 2099
>> 
>> typedef struct {
>> int width;
>> @@ -829,6 +833,9 @@ QemuCocoaView *cocoaView;
>> - (void)powerDownQEMU:(id)sender;
>> - (void)ejectDeviceMedia:(id)sender;
>> - (void)changeDeviceMedia:(id)sender;
>> +- (void)mountImageFile:(id)sender;
>> +- (void)ejectImageFile:(id)sender;
>> +- (void)updateEjectImageMenuItems;
>> @end
>> 
>> @implementation QemuCocoaAppController
>> @@ -1125,6 +1132,179 @@ QemuCocoaView *cocoaView;
>> }
>> }
>> 
>> +/* Displays a dialog box asking the user for an image file to mount */
>> +- (void)mountImageFile:(id)sender
>> +{
>> +/* Display the file open dialog */
>> +NSOpenPanel * openPanel;
>> +openPanel = [NSOpenPanel openPanel];
>> +[openPanel setCanChooseFiles: YES];
>> +[openPanel setAllowsMultipleSelection: NO];
>> +[openPanel setAllowedFileTypes: supportedImageFileTypes];
>> +if([openPanel runModal] == NSFileHandlingPanelOKButton) {
>> +NSString * file = [[[openPanel URLs] objectAtIndex: 0] path];
>> +if(file == nil) {
>> +NSBeep();
>> +QEMU_Alert(@"Failed to convert URL to file path!");
>> +return;
>> +}
>> +
>> +static int usbDiskCount;
>> +char *idString, *fileName, *driveOptions, *fileNameHint;
>> +
>> +/* Create the file name hint */
>> +NSString *stringBuffer;
>> +const int fileNameHintSize = 10;
>> +stringBuffer = [file lastPathComponent];
>> +stringBuffer = [stringBuffer stringByDeletingPathExtension];
>> +if([stringBuffer length] > fileNameHintSize) {
>> +stringBuffer = [stringBuffer substringToIndex: 
>> fileNameHintSize];
>> +}
>> +fileNameHint = g_strdup_printf("%s",
>> +[stringBuffer cStringUsingEncoding: 
>> NSASCIIStringEncoding]);
>> +
>> +idString = g_strdup_printf("%s_%s_%d", USB_DISK_ID, fileNameHint,
>> +
>> usbDiskCount++);
> 
> Indentation is off, long line.
I assume you are talking about the "usbDiskCount++);" part. Did you want it more
right justified?

> We don't use CamelCase for variable names in QEMU.  See CODING_STYLE,
> section 3. Naming.

Good catch. 

> 
> More of the same elsewhere.
> 
> Your system-generated ID can theoretically clash with a user-specified
> ID.  
The gui code does run at the start of QEMU. But the user could have specified
an ID at the command line. Given how long the auto-generated ID is, I think
there is little probability that would happen. I can make the auto-generated
ID impossible for the user to use by using a '#' character at the start. 

> We should reserve a name space for system-generated IDs.  Related:
> our discussion Re: Should we auto-generate IDs?  Anyway, wider issue,
> not something this patch can solve.
I don't like mixing topics in emails but what was the outcome of the fifth great
discussion on auto-generated ID's? Are you or another maintainer going to use
someone's patch?

> 
>> +
>> +fileName = g_strdup_printf("%s",
>> +[file cStringUsingEncoding: 
>> NSASCIIStringEncoding]);
>> +
>> +/* drive_add equivalent code */
>> +driveOptions = g_strdup_printf("if=none,id=%s,file=%s", idString,
>> + 
>> fileName);
>> +DriveInfo *dinfo;

[Qemu-devel] [PATCH v2] ui/cocoa.m: Add Mount image file menu item

[Programmingkid]

Add "Mount Image File..." and a "Eject Image File" menu items to
cocoa interface. This patch makes sharing files between the
host and the guest user-friendly.

The "Mount Image File..." menu item displays a dialog box having the
user pick an image file to use in QEMU. The image file is setup as
a USB flash drive. The user can do the equivalent of removing the
flash drive by selecting the file in the "Eject Image File" submenu.

Signed-off-by: John Arbuckle 

---
Detects if the user actually specified the -usb option.
Removed the sendMonitorCommand() function. 
Replaced a lot of code with C interface equivalent of monitor commands
drive_add and device_add. 

 ui/cocoa.m |  228 +++-
 1 files changed, 227 insertions(+), 1 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 334e6f6..df1faea 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -31,6 +31,8 @@
 #include "sysemu/sysemu.h"
 #include "qmp-commands.h"
 #include "sysemu/blockdev.h"
+#include "include/monitor/qdev.h"
+#include "hw/boards.h"
 
 #ifndef MAC_OS_X_VERSION_10_5
 #define MAC_OS_X_VERSION_10_5 1050
@@ -52,6 +54,8 @@
 #endif
 
 #define cgrect(nsrect) (*(CGRect *)&(nsrect))
+#define USB_DISK_ID "USB_DISK"
+#define EJECT_IMAGE_FILE_TAG 2099
 
 typedef struct {
 int width;
@@ -829,6 +833,9 @@ QemuCocoaView *cocoaView;
 - (void)powerDownQEMU:(id)sender;
 - (void)ejectDeviceMedia:(id)sender;
 - (void)changeDeviceMedia:(id)sender;
+- (void)mountImageFile:(id)sender;
+- (void)ejectImageFile:(id)sender;
+- (void)updateEjectImageMenuItems;
 @end
 
 @implementation QemuCocoaAppController
@@ -1125,6 +1132,179 @@ QemuCocoaView *cocoaView;
 }
 }
 
+/* Displays a dialog box asking the user for an image file to mount */
+- (void)mountImageFile:(id)sender
+{
+/* Display the file open dialog */
+NSOpenPanel * openPanel;
+openPanel = [NSOpenPanel openPanel];
+[openPanel setCanChooseFiles: YES];
+[openPanel setAllowsMultipleSelection: NO];
+[openPanel setAllowedFileTypes: supportedImageFileTypes];
+if([openPanel runModal] == NSFileHandlingPanelOKButton) {
+NSString * file = [[[openPanel URLs] objectAtIndex: 0] path];
+if(file == nil) {
+NSBeep();
+QEMU_Alert(@"Failed to convert URL to file path!");
+return;
+}
+
+static int usbDiskCount;
+char *idString, *fileName, *driveOptions, *fileNameHint;
+
+/* Create the file name hint */
+NSString *stringBuffer;
+const int fileNameHintSize = 10;
+stringBuffer = [file lastPathComponent];
+stringBuffer = [stringBuffer stringByDeletingPathExtension];
+if([stringBuffer length] > fileNameHintSize) {
+stringBuffer = [stringBuffer substringToIndex: fileNameHintSize];
+}
+fileNameHint = g_strdup_printf("%s",
+[stringBuffer cStringUsingEncoding: 
NSASCIIStringEncoding]);
+
+idString = g_strdup_printf("%s_%s_%d", USB_DISK_ID, fileNameHint,
+
usbDiskCount++);
+
+fileName = g_strdup_printf("%s",
+[file cStringUsingEncoding: 
NSASCIIStringEncoding]);
+
+/* drive_add equivalent code */
+driveOptions = g_strdup_printf("if=none,id=%s,file=%s", idString,
+ fileName);
+DriveInfo *dinfo;
+QemuOpts *opts;
+MachineClass *mc;
+
+opts = drive_def(driveOptions);
+if (!opts) {
+QEMU_Alert(@"Error: could not create QemuOpts object!");
+return;
+}
+
+mc = MACHINE_GET_CLASS(current_machine);
+dinfo = drive_new(opts, mc->block_default_type);
+if (!dinfo) {
+qemu_opts_del(opts);
+QEMU_Alert(@"Error: could not create DriveInfo object!");
+return;
+}
+
+/* device_add equivalent code */
+
+/* Create the QDict object */
+QDict * qdict;
+qdict = qdict_new();
+qdict_set_default_str(qdict, "driver", "usb-storage");
+qdict_set_default_str(qdict, "id", idString);
+qdict_set_default_str(qdict, "drive", idString);
+
+QObject *ret_data;
+ret_data = g_malloc(1); /* This is just to silence a warning */
+
+Error *errp = NULL;
+qmp_device_add(qdict, _data, );
+handleAnyDeviceErrors(errp);
+[self updateEjectImageMenuItems];
+
+g_free(qdict);
+g_free(fileName);
+g_free(idString);
+g_free(driveOptions);
+g_free(ret_data);
+}
+}
+
+/* Removes an image file from QEMU */
+- (void)ejectImageFile:(id) sender
+{
+NSString *imageFileID;
+
+imageFileID = [sender representedObject];
+if (imageFileID == nil) {
+NSBeep();
+QEMU_Alert(@"Could not find image file's ID!");
+return;
+}
+
+