On Sep 11, 2015, at 3:40 AM, Markus Armbruster wrote: Only reviewing around qmp_device_add() and such, ignoring the GUI part. Programmingkid < address@hidden> 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 <address@hidden>
---
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;
+ QemuOpts *opts;
+ MachineClass *mc;
No declarations in the middle of blocks, please. See CODING_STYLE section 5. Declarations.
Another good catch. More of the same elsewhere. +
+ opts = drive_def(driveOptions);
Breaks when fileName contains a comma.
Thanks for catching it. 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, "");
I tired using drive_add() so many times. It didn't work. Using 'info block' in the monitor is how I checked. 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 = drive_new(opts, mc->block_default_type);
+ if (!dinfo) {
+ qemu_opts_del(opts);
+ QEMU_Alert(@"Error: could not create DriveInfo object!");
+ return;
+ }
The real error messages will end up on stderr. The proper interface to use will be blockdev_init(), or maybe qmp_blockdev_add(). "Will be" because they're still being developed, and using them in this state might be too much to ask of you.
qmp_blockdev_add() has that BlockdevOptions type that doesn't look easy to use.
blockdev_init() looks usable. The only thing to find out is what does it expect to be in its QDict parameter.
There doesn't seem to be a problem with drive_new(). There is no indication that the function is depreciated.
+
+ /* 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);
Using the same ID for two different things happens to work at this time, but it's confusing, and best avoided.
Didn't know this was a problem. I just followed the example from here:
What do you suggest I use for "drive"?
+
+ QObject *ret_data;
+ ret_data = g_malloc(1); /* This is just to silence a warning */
Must be ret_data = NULL.
Excellent idea. +
+ Error *errp = NULL;
+ qmp_device_add(qdict, &ret_data, &errp);
+ handleAnyDeviceErrors(errp);
+ [self updateEjectImageMenuItems];
+
+ g_free(qdict);
+ g_free(fileName);
+ g_free(idString);
+ g_free(driveOptions);
+ g_free(ret_data);
You leak most of these on your error returns.
A goto statement will fix this. Didn't check whether you leak anything here.
+ }
+}
+
+/* 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;
+ }
+
+ Error *errp = NULL;
+ qmp_device_del([imageFileID cStringUsingEncoding: NSASCIIStringEncoding],
+ &errp);
+ handleAnyDeviceErrors(errp);
+ [self updateEjectImageMenuItems];
+}
+
+/* Gives each mounted image file an eject menu item */
+- (void) updateEjectImageMenuItems
+{
+ NSMenu *machineMenu;
+ machineMenu = [[[NSApp mainMenu] itemWithTitle:@"Machine"] submenu];
+
+ /* Remove old menu items*/
+ NSMenu * ejectSubmenu;
+ ejectSubmenu = [[machineMenu itemWithTag: EJECT_IMAGE_FILE_TAG] submenu];
+ if(!ejectSubmenu) {
+ NSBeep();
+ QEMU_Alert(@"Failed to find eject submenu!");
+ return;
+ }
+ int index;
+ for (index = 0; index < [ejectSubmenu numberOfItems]; index++) {
+ [ejectSubmenu removeItemAtIndex: 0];
+ }
+ /* Needed probably because of a bug with cocoa */
+ if ([ejectSubmenu numberOfItems] > 0) {
+ [ejectSubmenu removeItemAtIndex: 0];
+ }
+
+ BlockInfoList *currentDevice;
+ currentDevice = qmp_query_block(NULL);
+
+ NSString *fileName, *deviceName;
+ NSMenuItem *ejectFileMenuItem; /* Used with each mounted image file */
+
+ /* Look for mounted image files */
+ while(currentDevice) {
+ if (!currentDevice->value || !currentDevice->value->inserted
+ || !currentDevice->value->inserted->file) {
+ currentDevice = currentDevice->next;
+ continue;
+ }
+
+ /* if the device's name is the generated ID */
+ if (!strstr(currentDevice->value->device, USB_DISK_ID)) {
+ currentDevice = currentDevice->next;
+ continue;
+ }
+
+ fileName = [NSString stringWithFormat: @"%s", currentDevice->value->inserted->file];
+ fileName = [fileName lastPathComponent]; /* To obtain only the file name */
+
+ ejectFileMenuItem = [[NSMenuItem alloc] initWithTitle: [NSString stringWithFormat: @"Eject %@", fileName]
+ action: @selector(ejectImageFile:)
+ keyEquivalent: @""];
+ [ejectSubmenu addItem: ejectFileMenuItem];
+ deviceName = [NSString stringWithFormat: @"%s", currentDevice->value->device];
+ [ejectFileMenuItem setRepresentedObject: deviceName];
+ [ejectFileMenuItem autorelease];
+ currentDevice = currentDevice->next;
+ }
+
+ /* Add default menu item if submenu is empty */
+ if([ejectSubmenu numberOfItems] == 0) {
+
+ /* Create the default menu item */
+ NSMenuItem *emptyMenuItem;
+ emptyMenuItem = [NSMenuItem new];
+ [emptyMenuItem setTitle: @"No items available"];
+ [emptyMenuItem setEnabled: NO];
+
+ /* Add the default menu item to the submenu */
+ [ejectSubmenu addItem: emptyMenuItem];
+ [emptyMenuItem release];
+ }
+}
+
@end
@@ -1338,10 +1518,52 @@ static void add_console_menu_entries(void)
}
}
+/* Determines if USB is available */
+static bool usbAvailable(void)
+{
+ /* Look thru all the arguments sent to QEMU for '-usb' */
+ int index;
+ for (index = 0; index < gArgc; index++) {
+ if(strcmp(gArgv[index], "-usb") == 0) {
+ return true;
+ }
+ }
+ return false;
+}
-usb is just one way to enable USB. There's also --usb, and variations on --machine usb=on. USB may even be enabled by default. You need to search QOM for USB sockets.
Do you know of example code I could study that does this? +
+/* Adds menu items that can mount an image file like a usb flash drive */
+static void addImageMountingMenus(NSMenu *menu)
+{
+ [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Mount Image File..."
+ action: @selector(mountImageFile:) keyEquivalent: @""] autorelease]];
+
+ /* Create the eject menu item */
+ NSMenuItem *ejectMenuItem;
+ ejectMenuItem = [NSMenuItem new];
+ [ejectMenuItem setTitle: @"Eject Image File"];
+ [ejectMenuItem setTag: EJECT_IMAGE_FILE_TAG];
+ [menu addItem: ejectMenuItem];
+ [ejectMenuItem autorelease];
Okay, one drive-by GUI remark: We eject media, we unplug devices. Ejecting image files makes no sense to me :)
I think eject conveys the message of removing something from the system. Just thinking about a menu item called "unplug" makes me cringe. +
+ /* Create the default menu item for the eject menu item's submenu*/
+ NSMenuItem *emptyMenuItem;
+ emptyMenuItem = [NSMenuItem new];
+ [emptyMenuItem setTitle: @"No items available"];
+ [emptyMenuItem setEnabled: NO];
+ [emptyMenuItem autorelease];
+
+ /* Add the default menu item to the submenu, the "No items" menu item */
+ NSMenu *submenu;
+ submenu = [NSMenu new];
+ [ejectMenuItem setSubmenu: submenu];
+ [submenu addItem: emptyMenuItem];
+ [submenu autorelease];
+}
+
/* Make menu items for all removable devices.
* Each device is given an 'Eject' and 'Change' menu item.
*/
-static void addRemovableDevicesMenuItems()
+static void addRemovableDevicesMenuItems(void)
{
NSMenu *menu;
NSMenuItem *menuItem;
@@ -1402,6 +1624,10 @@ static void addRemovableDevicesMenuItems()
currentDevice = currentDevice->next;
}
qapi_free_BlockInfoList(pointerToFree);
+
+ if(usbAvailable() == true){
+ addImageMountingMenus(menu);
+ }
}
void cocoa_display_init(DisplayState *ds, int full_screen)
Thank you very much for helping.
|