On Sep 11, 2015, at 3:40 AM, Markus Armbruster wrote:
> 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.
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:
https://wiki.ubuntu.com/QemuDiskHotplug#Hotplug_USB_Disk]
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.
