[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v4 10/11] qemu-img: Set the ID of the block job

From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH v4 10/11] qemu-img: Set the ID of the block job in img_commit()
Date: Wed, 06 Jul 2016 14:07:35 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

On Tue 05 Jul 2016 05:56:09 PM CEST, Max Reitz <address@hidden> wrote:
>> @@ -132,12 +132,6 @@ void *block_job_create(const char *job_id, const 
>> BlockJobDriver *driver,
>>      if (job_id == NULL) {
>>          job_id = bdrv_get_device_name(bs);
>> -        /* Assign a default ID if the BDS does not have a device
>> -         * name. We'll get rid of this soon when we finish extending
>> -         * the API of all commands that create block jobs. */
>> -        if (job_id[0] == '\0') {
>> -            job_id = "default_job";
>> -        }
> I stand by my R-b, but as a remark to what you said for v3: I can't
> imagine how this function can be called in a way that job_id will be
> empty here (after this patch), and this is why I proposed an
> assert(job_id[0] != '\0'). It'd probably be a mistake on our part if
> such a case could happen (which is why an assertion would be fine).

At the moment you cannot do it, but if we allow creating block jobs on
other nodes (e.g. intermediate block streaming) the user must provide a
job ID, else there'll be no default.

> However, gracefully returning an error is of course fine, too. The
> issue I take with this is that the error we'd be returning is "Invalid
> job ID ''", which isn't very helpful (it should be "No job ID
> specified, and no default available").

We can add a special check for the empty string and use that error
message that you propose.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]