Only reviewing around qmp_device_add() and such, ignoring the GUI part.
Programmingkid <programmingk...@gmail.com> 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 <programmingk...@gmail.com>
>
> ---
> 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 = 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.
> +
> + /* 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.
> +
> + QObject *ret_data;
> + ret_data = g_malloc(1); /* This is just to silence a warning */
Must be ret_data = NULL.
> +
> + 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.
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.
> +
> +/* 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 :)
> +
> + /* 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)
