Re: [Qemu-devel] [PATCH v5] ui/cocoa.m: Machine menu patch for Mac OS X

[Peter Maydell]

On 17 February 2015 at 04:55, Programmingkid <address@hidden> wrote:
> Added 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.
> Pause and resume menu items with the word paused displayed on the window.
>
> Signed-off-by: John Arbuckle <address@hidden>

This needs to be split up into a multipatch series,
because it's doing too many things at once. Probably
something like:
 * powerdown/reset/pause/resume menu entries
 * display 'paused' on window when paused
 * floppy/cd menu items

> +/* Determine if the current emulator has a floppy drive */
> +static bool emulatorHasFloppy()
> +{
> +    if (emulatorHasDevice("floppy", floppy_drive_name)) {
> +        return true;
> +    } else {
> +        return false;
> +    }
> +}
> +
> +/* Determine if the current emulator has a CDROM drive */
> +static bool emulatorHasCDROM()
> +{
> +    if (emulatorHasDevice("cd", cdrom_drive_name)) {
> +        return true;
> +    } else {
> +        return false;
> +    }
> +}
> +
> +/* Determines if the given device is a floppy drive */
> +static bool isFloppyDevice(BlockInfo * current_device)
> +{
> +    if(strstr(current_device->device, "floppy")) {
> +        return true;
> +    } else {
> +        return false;
> +    }
> +
> +}
> +
> +/* Determines if the given device is a CD drive */
> +static bool isCdromDevice(BlockInfo * current_device)
> +{
> +    if(strstr(current_device->device, "-cd")) {
> +        return true;
> +    } else {
> +        return false;
> +    }
> +}
> +
> +/* Returns a floppy device */
> +static NSString * getFloppyDevice(int index)
> +{
> +    int count = 0;
> +    BlockInfoList *current_device;
> +    current_device = qmp_query_block(false);
> +    if(current_device == NULL) {
> +        NSBeep();
> +        NSRunAlertPanel(@"Alert", @"Could not query block devices!", @"OK", 
> nil, nil);
> +        printf("Error: could not query block devices!\nFunction: 
> getFloppyDevice()\n");
> +        return @"FAILED TO QUERY FOR FLOPPY DRIVES";
> +    }
> +
> +    // look thru all the devices
> +    while (current_device) {
> +        if(isFloppyDevice(current_device->value)) {  /* If found a floppy 
> drive */
> +            if(count == index) { /* The drive we want */
> +                return [NSString stringWithFormat: @"%s", 
> current_device->value->device];
> +            }
> +            count++;
> +        }
> +        current_device = current_device->next;
> +    }
> +
> +    /* If failed to find the drive */
> +    NSRunAlertPanel(@"Alert", @"Could not find floppy drive.", @"OK", nil, 
> nil);
> +    printf("Error: No floppy drive found at index %d\n\a", index);
> +    return [NSString stringWithFormat: @"NO FLOPPY DRIVE FOUND AT INDEX %d", 
> index ];
> +}
> +
> +/* Returns a cdrom device */
> +static NSString * getCdromDevice(int index)
> +{
> +    int count = 0;
> +    BlockInfoList *current_device;
> +    current_device = qmp_query_block(false);
> +    if(current_device == NULL) {
> +        NSBeep();
> +        NSRunAlertPanel(@"Alert", @"Could not query block devices!", @"OK", 
> nil, nil);
> +        printf("Error: could not query block devices!\nFunction: 
> getCdromDevice()");
> +        return @"FAILED TO QUERY FOR CDROM DRIVES";
> +    }
> +
> +    // look thru all the devices
> +    while (current_device) {
> +        if(isCdromDevice(current_device->value)) {  /* If found a cd drive */
> +            if(count == index) { /* The drive we want */
> +                return [NSString stringWithFormat: @"%s", 
> current_device->value->device];
> +            }
> +            count++;
> +        }
> +        current_device = current_device->next;
> +    }
> +
> +    /* If failed to find the drive */
> +    NSRunAlertPanel(@"Alert", @"Could not find cdrom.", @"OK", nil, nil);
> +    printf("Error: could not find cdrom drive.\n");
> +    return [NSString stringWithFormat: @"NO CDROM DRIVE FOUND AT INDEX %d", 
> index ];
> +}

You seem to have a lot of functions which look very similar
to each other, which surely could be refactored to avoid
the duplication.

thanks
-- PMM