From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50069 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pphfm-0001Fw-9g for qemu-devel@nongnu.org; Wed, 16 Feb 2011 08:44:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pphfk-0002nG-PB for qemu-devel@nongnu.org; Wed, 16 Feb 2011 08:44:02 -0500 Received: from mail-qy0-f173.google.com ([209.85.216.173]:54304) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pphfk-0002nA-Kv for qemu-devel@nongnu.org; Wed, 16 Feb 2011 08:44:00 -0500 Received: by qyl38 with SMTP id 38so3788622qyl.4 for ; Wed, 16 Feb 2011 05:43:59 -0800 (PST) Message-ID: <4D5BD48B.6010705@codemonkey.ws> Date: Wed, 16 Feb 2011 07:43:39 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [RFC] qapi: events in QMP References: <4D581E04.1020901@codemonkey.ws> <4D58FADB.3010005@redhat.com> <4D591A01.4030105@codemonkey.ws> <4D5920ED.6020104@redhat.com> <20110214104517.32b77291@doriath> <4D593E8F.7050306@codemonkey.ws> <20110214163443.57ad8a37@doriath> <4D5983B3.5010902@codemonkey.ws> <4D5A4541.3080906@redhat.com> <20110215113831.6d647b95@doriath> <4D5B217A.40202@codemonkey.ws> <4D5B8FCC.3050303@redhat.com> In-Reply-To: <4D5B8FCC.3050303@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Luiz Capitulino , qemu-devel , Markus Armbruster On 02/16/2011 02:50 AM, Kevin Wolf wrote: > Am 16.02.2011 01:59, schrieb Anthony Liguori: > >> On 02/15/2011 07:38 AM, Luiz Capitulino wrote: >> >>> On Tue, 15 Feb 2011 10:20:01 +0100 >>> Kevin Wolf wrote: >>> >>> >>> >>>> Am 14.02.2011 20:34, schrieb Anthony Liguori: >>>> >>>> >>>>> On 02/14/2011 12:34 PM, Luiz Capitulino wrote: >>>>> >>>>> >>>>>> On Mon, 14 Feb 2011 08:39:11 -0600 >>>>>> Anthony Liguori wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> On 02/14/2011 06:45 AM, Luiz Capitulino wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>>> So the question is: how does the schema based design support extending >>>>>>>> commands or events? Does it require adding new commands/events? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> Well, let me ask you, how do we do that today? >>>>>>> >>>>>>> Let's say that I want to add a new parameter to the `change' function so >>>>>>> that I can include a salt parameter as part of the password. >>>>>>> >>>>>>> The way we'd do this today is by checking for the 'salt' parameter in >>>>>>> qdict, and if it's not present, use a random salt or something like that. >>>>>>> >>>>>>> >>>>>>> >>>>>> You likely want to do what you did before. Of course that you have to >>>>>> consider if what you're doing is extending an existing command or badly >>>>>> overloading it (like change is today), in this case you'll want to add >>>>>> a new command instead. >>>>>> >>>>>> But yes, the use-case here is extending an existing command. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> However, if I'm a QMP client, how can I tell whether you're going to >>>>>>> ignore my salt parameter or actually use it? Nothing in QMP tells me >>>>>>> this today. If I set the salt parameter in the `change' command, I'll >>>>>>> just get a success message. >>>>>>> >>>>>>> >>>>>>> >>>>>> I'm sorry? >>>>>> >>>>>> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234", "salt": "r1" } } >>>>>> {"error": {"class": "InvalidParameter", "desc": "Invalid parameter 'salt'", "data": {"name": "salt"}}} >>>>>> >>>>>> >>>>>> >>>>> So I'm supposed to execute the command, and if execution fails, drop the >>>>> new parameter? If we add a few optional parameters, does that mean I >>>>> have to try every possible combination of parameters? >>>>> >>>>> >>>> How is that different from trying out multiple commands? In the end, you >>>> always need some meta information like a schema in order to avoid trying >>>> out which parameters the server supports. >>>> >>>> Anyway, I think there's a second interesting point: Adding parameters >>>> does cause these problems, but it's different for data sent from qemu to >>>> the client (return values and events). If we add more information there, >>>> an older client can just ignore it, without even looking at a schema. >>>> >>>> So I think we should consider this for return values and definitely do >>>> it for events. Sending out five different messages for a single event >>>> that are completely redundant and only differ in the number of fields is >>>> just insane (okay, they wouldn't actually get on the wire because a >>>> client registers only for one of them, but the code for generating them >>>> must exist). >>>> >>>> >>> That's my point when I asked about events in the other thread. >>> >>> >> Okay, I had confused myself about this. It's not quite as bad as I had >> been saying. >> >> One of the reasons to have generated allocation function is so that we >> can make sure to always pad structures. Since all optional fields has a >> bool to indicate the fields presence, by setting the allocated structure >> to zero, we can support forwards compatibility for structures. >> > I think in most cases we would even get away with a default value > instead of the bool. For example for strings, NULL would be a very clear > indication that the field wasn't there in the JSON message. In order to support forwards compatibility, we need to have a zero-value for non-presence. For strings or pointers, NULL would work very well. But for integers, I'm not sure we can reasonably use 0 as a universal default value. We could just use has_ fields for non-pointers but I figured consistency would make the code more robust since it's hard to tell that a field is really optional vs. required. For instance: typedef struct BlockInfo { const char *device_name; bool has_backing_file; const char *backing_file; } BlockInfo; It's very obvious that backing_file is optional. If you don't set has_backing_file (because you're incorrectly treating backing_file is required), it will fail immediately as the field won't be marshalled. OTOH: typedef struct BlockInfo { const char *device_name; // optional const char *backing_file; } BlockInfo; Is a bit easier to screw up. If you happen to not do the NULL check and work with a client that's sending it, you can end up with a NULL pointer dereference pretty easily. >> If we expect to add fields later, we just have to make sure we use a >> structure to encapsulate things. >> > As stated before, I think we should use structures for all events. I > still don't understand why we should have an exception for events. Any > other command returns structures, too, and you don't automagically pull > their fields up one level anywhere except for events. > That's not entirely true. For human-monitor-command, we return a bare string. For all other commands, we return structures specifically to enable better forwards compatibility. I do agree that for most of our events, we should be using a structure for passing information. That's not what we're doing today but there's only a couple events that are even doing that so fixing it won't be that bad. Regards, Anthony Liguori > Kevin > >