From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50125) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCC03-0005pZ-Ld for Qemu-devel@nongnu.org; Mon, 26 Mar 2012 11:38:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SCBzv-0007me-Ot for Qemu-devel@nongnu.org; Mon, 26 Mar 2012 11:38:27 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:52934) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCBzv-0007ln-Iw for Qemu-devel@nongnu.org; Mon, 26 Mar 2012 11:38:19 -0400 Received: by yenr5 with SMTP id r5so4819721yen.4 for ; Mon, 26 Mar 2012 08:38:17 -0700 (PDT) Message-ID: <4F708D66.9010509@codemonkey.ws> Date: Mon, 26 Mar 2012 10:38:14 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <4F702B56.8030400@redhat.com> <4F707118.1070200@codemonkey.ws> <4F708673.1070503@redhat.com> <4F7087C9.6020205@codemonkey.ws> <4F708C85.5090606@redhat.com> In-Reply-To: <4F708C85.5090606@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] Ignoring errno makes QMP errors suck List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Qemu-devel@nongnu.org, Luiz Capitulino On 03/26/2012 10:34 AM, Kevin Wolf wrote: > Am 26.03.2012 17:14, schrieb Anthony Liguori: >> On 03/26/2012 10:08 AM, Kevin Wolf wrote: >>> Am 26.03.2012 15:37, schrieb Anthony Liguori: >>>> On 03/26/2012 03:39 AM, Kevin Wolf wrote: >>>>> Hi, >>>>> >>>>> I keep getting reports of problems, with nice error descriptions that >>>>> usually look very similar to what I produced here: >>>>> >>>>> {"execute":"blockdev-snapshot-sync","arguments":{"device":"ide0-hd0","snapshot-file":"/tmp/backing.qcow2"}} >>>>> {"error": {"class": "OpenFileFailed", "desc": "Could not open >>>>> '/tmp/backing.qcow2'", "data": {"filename": "/tmp/backing.qcow2"}}} >>>> >>>> This is not QMP's fault. This is the block layers. Specifically, you're missing: >>>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 1a500b8..04c3a39 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -777,7 +777,11 @@ void qmp_transaction(BlockdevActionList *dev_list, Error ** >>>> states->old_bs->drv->format_name, >>>> NULL, -1, flags); >>>> if (ret) { >>>> - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >>>> + if (ret == -EPERM) { >>>> + error_set(errp, QERR_PERMISSION_DENIED); >>>> + } else { >>>> + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >>>> + } >>>> goto delete_and_fail; >>>> } >>>> } >>>> >>>> Which is handling: >>>> >>>> ret = bdrv_img_create(new_image_file, format, >>>> states->old_bs->filename, >>>> states->old_bs->drv->format_name, >>>> NULL, -1, flags); >>> >>> It really should be something like this: >>> >>> - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >>> + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file, -ret); >>> >>> And QERR_OPEN_FILE_FAILED would contain a conversion specifier for >>> errnos in qobject_from_jsonv(). >> >> No, it really shouldn't be. >> >> Errors are verbs, not knows, you're treating the error as a noun "the operation >> open file" and looking to use errno as the verb. This is wrong. The noun is >> implied in the operation. >> >> You could use error_set_from_errno(errp, -ret) which doesn't exist, but could. >> But errno on it's own lacks a lot of useful information so I wouldn't suggest >> always using such a function. > > I couldn't care less about nouns and verbs and stuff. > > I want to transfer the information that a "permission denied" error has > happened and on which file it has happened. The existing OpenFileFailed > error doesn't allow to specify that the missing permission was the > problem, and a hypothetical PermissionDenied error wouldn't allow me to > specify the file name because it would be too generic. > > This is my problem, and nothing else. Then extend PermissionDenied to include a filename. Problem solved. >>> Yes, but that's a completely independent problem. >> >> It's not really. If you want high quality errors, you have to push the error >> handling up the stack. That's the reason we have Error--to introduce a common >> error handling framework capable of generating high quality error information. > > Yes, but if there is no appropriate error, then even if I added Error > support to the Linux syscalls they couldn't generate the right error > message. This is why I still think it's completely independent. Than add/improve the existing errors. >>>> QMP provides all the infrastructure you need. You just have to use it. >>> >>> It doesn't provide the portable way of reporting errno yet. >> >> I think what you'll find is that 90% of the time, the errno is being generated >> somewhere within QEMU code or that there's a system call that returns on one >> errno that we care about. If you push error handling down to the source of the >> error, I'm sure you'll find that you almost never have to switch on errno. > > I'm looking for a solution that works now and not only in five years > when all of qemu has been rewritten. I'm also not quite sure if we > really want to drag Errors through coroutines and AIO code in the block > layer... You can bridge this all today. I showed you in patches how to do it. It's not difficult. >> Having an error_set_from_errno() would be a stop-gap to help bridge unconverted >> code, but if you want high quality errors, the right answer is to convert the >> existing code to use the Error infrastructure properly. > > Only if it can be used properly. That is, if I can somehow create an > error message that contains _both_ the file name and the error cause. You can add parameters to Errors in a fully compatible fashion, so just add an filename parameter to PermissionDenied. Problem solved. Regards, Anthony Liguori