Re: [Qemu-devel] [PATCH] ui/cocoa.m: Add Mount image file menu item

[Markus Armbruster]

Programmingkid <address@hidden> writes:

> On Sep 8, 2015, at 2:46 PM, Markus Armbruster wrote:
>
>> Programmingkid <address@hidden> writes:
>> 
>>> On Sep 8, 2015, at 12:17 PM, Peter Maydell wrote:
>>> 
>>>> On 2 September 2015 at 01:56, Programmingkid
>>>> <address@hidden> wrote:
>>>>> 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>
>>>>> 
>>>>> ---
>>>>> ui/cocoa.m |  212
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>> 1 files changed, 210 insertions(+), 1 deletions(-)
>>>>> 
>>>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>>>> index 334e6f6..6c0ec18 100644
>>>>> --- a/ui/cocoa.m
>>>>> +++ b/ui/cocoa.m
>>>>> @@ -52,6 +52,9 @@
>>>>> #endif
>>>>> 
>>>>> 
>>>>> 
>>>>> #define cgrect(nsrect) (*(CGRect *)&(nsrect))
>>>>> +#define USB_DISK_ID "USB_DISK"
>>>>> +#define EJECT_IMAGE_FILE_TAG 2099
>>>>> 
>>>>> 
>>>>> 
>>>>> typedef struct {
>>>>>    int width;
>>>>> @@ -263,6 +266,43 @@ static void handleAnyDeviceErrors(Error * err)
>>>>>    }
>>>>> }
>>>>> 
>>>>> 
>>>>> 
>>>>> +/* Sends a command to the monitor console */
>>>>> +static void sendMonitorCommand(const char * commandString)
>>>>> +{
>>>>> +    int index;
>>>>> +    char * consoleName;
>>>>> +    static QemuConsole *monitor;
>>>>> +
>>>>> +    /* If the monitor console hasn't been found yet */
>>>>> +    if(!monitor) {
>>>>> +        index = 0;
>>>>> +        /* Find the monitor console */
>>>>> +        while (qemu_console_lookup_by_index(index) != NULL) {
>>>>> +            consoleName =
>>>>> qemu_console_get_label(qemu_console_lookup_by_index(index));
>>>>> +            if(strstr(consoleName, "monitor")) {
>>>>> +                monitor = qemu_console_lookup_by_index(index);
>>>>> +                break;
>>>>> +            }
>>>>> +            index++;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    /* If the monitor console was not found */
>>>>> +    if(!monitor) {
>>>>> +        NSBeep();
>>>>> +        QEMU_Alert(@"Failed to find the monitor console!");
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    /* send each letter in the commandString to the monitor */
>>>>> +    for (index = 0; index < strlen(commandString); index++) {
>>>>> +        kbd_put_keysym_console(monitor, commandString[index]);
>>>>> +    }
>>>> 
>>>> We're doing this by sending a string to the human monitor?
>> 
>> No way :)
>> 
>> You should not send a string to a monitor (QMP or HMP) just because you
>> can't be bothered to look up the proper C interfaces.
>
> I wouldn't say it like that. I did try to find the functions I needed,
> but didn't succeed.

I shouldn't have said "just because you can't be bothered", since I
don't know whether you tried.  My apologies.

What I needed to tell you was that this method is not going to be
accepted, preferrably before you sink more of your time into it.

My first try[1]: "Sure you want to do that, and not call the C interface
instead?"  This is the exceedingly polite version (for a German) of "bad
idea, drop it".  Followed by an explanation how to find the C interface
to use.  No reply.

Second try[2]: "On the flip side, you'll never get a patch abusing
handle_hmp_command() or handle_qmp_command() as internal interface
committed."  This was in context of you discussing the bad idea further,
with Max.  No reply to that part.  Okay, still not blunt enough.

Third try[3]: "No way :)".  Blunt enough to get the point across?

> Didn't you say yourself you don't want to see gui patches that change
> other code?
> My research indicated there would have to be changes to other files if
> I did try to use
> the C interface functions.  
>
> For example, the function add_init_drive() in device-hotplug.c looked
> really good. The
> problem was that it is a static function. Since you don't want changes
> made to other
> files, I decided not to use it. 

Let me clarify my attitude to integrated GUIs once more:

1. We shouldn't do it (my opinion, not the project's, obviously).

2. As long as we do it anyway, it shouldn't interfere with our two core
   missions: user space component for hypervisors, machine emulator.

2a. It shouldn't mess up core code.

2b. It shouldn't waste time of core contributors.

Now, the fact that I've been wasting my time (and my employer's money)
by trying to teach you finding the proper interfaces should show that 2b
isn't absolute.  More like guidelines, anyway.  More like "shouldn't
waste too much time".  Likewise 2a: simple changes can be okay.  If not,
review will lead you to how it needs to be done instead.

See below for advice on add_init_drive().

>>>> That definitely doesn't seem like the right way to do this
>>>> (and there might not even be a human monitor to talk to)...
>>> 
>>> Under what situation is the human monitor not available? 
>>> 
>>> Would you know what function I should use in place of the commands the
>>> patch uses?
>> 
>> I explained that already for QMP:
>> http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00008.html
>> 
>> The mapping from HMP to the C interfaces can be more complex.  Going
>> from QMP to C is easier.
>
> The problem with QMP is that it is so difficult to use. It could be
> made to be more
> user-friendly. I will try to use it anyways in my next patch. Thank you.

Going from QMP to C is easier, because QMP commands should always be
straightforward wrappers around an internal C interface you can use.

Of course, you can try to find the proper C interfaces from HMP, too.
Find the HMP command handler in hmp-commands.hx.  Drill down until you
hit the internal interface that is also used by QMP.  In simple cases,
it's obvious, e.g. hmp_drive_backup() calls qmp_drive_backup().
However, there are cases where the HMP command does non-trivial
HMP-specific stuff, and you have to find your way through it to the
proper internal interface.  Worse, there are a few hairy cases left
where HMP still isn't what it should be, namely a wrapper around the
internal C interface for QMP.

Creating block backends is one of these.  At this time, add_init_drive()
is just a hotplug helper function, not an internal interface.  The
internal interfaces are further down: blockdev_init().  This is one
layer below the internal C interface for QMP qmp_blockdev_add(), because
the HMP commands behave differently for historical reasons (read:
they're messed up, but we won't change them until the ongoing work on
the QMP commands is complete).


[1] Subject: Re: [Qemu-devel] function to execute qmp commands
Date: Tue, 01 Sep 2015 08:19:02 +0200
Message-ID: <address@hidden>

[2] Subject: Re: [Qemu-devel] Mount image file feature
Date: Thu, 03 Sep 2015 11:46:29 +0200
Message-ID: <address@hidden>

[3] Subject: Re: [Qemu-devel] [PATCH] ui/cocoa.m: Add Mount image file
menu item
Date: Tue, 08 Sep 2015 20:46:02 +0200
Message-ID: <address@hidden>