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

[Programmingkid]

On Sep 9, 2015, at 3:29 AM, Markus Armbruster wrote:

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

Thank you very much for caring. I appreciate all the help I can receive. I so 
like my idea of
sending a command to QEMU as if the user typed it himself. It is so easy to 
maintain. So 
easy to use. So expandable. But given that two maintainers have told me that I 
can't do this,
the idea has to be abandoned. 

The C interface idea sounds good, but trying to figure out how to make any of 
the handler
functions work is very difficult. Just trying to make the QDict and QObject 
variables is just
too much. It needs to be a lot easier than this.

That leaves QMP. I am trying to figure it out. This is my attempt so far:

Error **errp;
char *commandBuffer;
commandBuffer = g_strdup_printf("{ \"execute\": \"quit\" }");
qmp_query_command_line_options(false, commandBuffer, errp);
printf("Program should quit now\n");

For some reason it didn't work. What am I doing wrong? Examples would really 
help.