Re: [Qemu-devel] [PATCH v2] Machine menu patch for Mac OS X

[Peter Maydell]

On 20 January 2015 at 17:22, Programmingkid <address@hidden> wrote:
> Features:
> Menu items to switch floppy and CD image files.
> Menu items to eject floppy and CD image files.
> Menu item to use /dev/cdrom.
> Verifies with the user before quitting QEMU by displaying a dialog box.
>
> Signed-off-by: John Arbuckle <address@hidden>

I get lots of compile warnings with this, including about
functions deprecated back in OSX 10.4, format string security
issues, and discarding of const qualifiers. These all need to
be fixed.

>
>  int gArgc;
>  char **gArgv;
> +#define MAX_DEVICE_NAME_SIZE 10
> +char floppy_drive_name[MAX_DEVICE_NAME_SIZE], 
> cdrom_drive_name[MAX_DEVICE_NAME_SIZE];
> +NSTextField * pause_label;

> +/*
> +Determine if the current emulator has the specified device.
> +device_name: the name of the device you want: floppy, cd
> +official_name: QEMU's name for the device: floppy0, ide-cd0
> +*/
> +static bool emulatorHasDevice(char * device_name, char * official_name)
> +{
> +    BlockInfoList * block_device_data;
> +    block_device_data = qmp_query_block(false);
> +    if(block_device_data == NULL) {
> +        return false;
> +    }
> +    while(block_device_data->next != NULL) {
> +        /* If we found the device */
> +        if (strstr(block_device_data->value->device, device_name)) {
> +            strcpy(official_name, block_device_data->value->device);

This is doing an unchecked strcpy into a fixed size buffer;
don't ever do this...

> +            qapi_free_BlockInfoList(block_device_data);
> +            return true;
> +        }
> +        block_device_data = block_device_data->next;
> +    }
> +    return false;
> +}

> +/* Adds the Machine menu to the menu bar. */
> +/* Has to be added separately because QEMU needs
> +   to be running to determine used devices.
> +*/
> +static void createMachineMenu()
> +{
> +    NSMenu * menu;
> +    NSMenuItem * menuItem;
> +
> +    // Machine menu
> +     menu = [[NSMenu alloc] initWithTitle: @"Machine"];
> +    [menu setAutoenablesItems: NO];
> +    [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Pause" action: 
> @selector(pauseQemu:) keyEquivalent: @""] autorelease]];
> +    [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Resume" action: 
> @selector(resumeQemu:) keyEquivalent: @""] autorelease]];

I suggest you split this patch up. The addition of 'pause'/'resume'
is straightforward because we already do that in the GTK front end.
Adding support for 'eject floppy/cdrom' is a separate feature and
should be in its own patch for review (you should cc the block
maintainers on that to check that we have the right mechanism for
"find the cdrom drive").

thanks
-- PMM