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 <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.

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)