From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0uw3-0004UD-5b for qemu-devel@nongnu.org; Fri, 24 Feb 2012 08:11:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S0uvy-00078y-JF for qemu-devel@nongnu.org; Fri, 24 Feb 2012 08:11:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41751) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0uvy-00078T-CI for qemu-devel@nongnu.org; Fri, 24 Feb 2012 08:11:38 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1ODBaQC005355 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 24 Feb 2012 08:11:36 -0500 Message-ID: <4F478C83.9050109@redhat.com> Date: Fri, 24 Feb 2012 14:11:31 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1329930815-7995-1-git-send-email-fsimonce@redhat.com> <1330083459-13583-2-git-send-email-fsimonce@redhat.com> <4F477C7C.4050400@redhat.com> In-Reply-To: <4F477C7C.4050400@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] Add the blockdev-reopen and blockdev-migrate commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Markus Armbruster , Federico Simoncelli , Marcelo Tosatti , qemu-devel@nongnu.org, lcapitulino@redhat.com On 02/24/2012 01:03 PM, Kevin Wolf wrote: >> + loop: >> + if (!(i < sizeof(filename) - 2)) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "new-image-file", "shorter filename"); >> + return; >> + } >> + >> + if (new_image_file[j] == ':' || new_image_file[j] == '\\') { > > Markus suggested that using comma for the separator is better even > though it requires escaping. It would allow to parse the option string > with QemuOpts. Isn't that a bit overengineering for an internal interface? >> + if (!escape) { >> + filename[i++] = '\\', escape = 1; >> + goto loop; >> + } else { >> + escape = 0; >> + } >> + } >> + >> + filename[i++] = new_image_file[j]; >> + } > > Looks like a string helper for qemu-option.c (it contains the parser, so > keeping the escaping nearby would make sense). > >> + >> + if (i + strlen(destination) + 2 > sizeof(filename)) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + "destination", "shorter filename"); >> + return; >> + } >> + >> + filename[i++] = ':'; >> + pstrcpy(filename + i, sizeof(filename) - i - 2, destination); >> + >> + change_blockdev_image(device, filename, "blkmirror", false, errp); >> + } else if (mode == BLOCK_MIGRATE_OP_STREAM) { >> + error_set(errp, QERR_NOT_SUPPORTED); > > Why even define it then? Because it's the default for the HMP. Until we have live merging, mirror mode cannot be consider anything more than a hack. >> ## >> +# @blockdev-reopen >> +# >> +# Assigns a new image file to a device. >> +# >> +# @device: the name of the device for which we are chainging the image file. >> +# >> +# @new-image-file: the target of the new image. If the file doesn't exists the >> +# command will fail. >> +# >> +# @format: #optional the format of the new image, default is 'qcow2'. >> +# >> +# Returns: nothing on success >> +# If @device is not a valid block device, DeviceNotFound >> +# If @new-image-file can't be opened, OpenFileFailed >> +# If @format is invalid, InvalidBlockFormat >> +# >> +# Since 1.1 >> +## >> + >> +{ 'command': 'blockdev-reopen', >> + 'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } } > > Same consideration on the name. > > Also I think we should immediately mark the command as deprecated (I > think there is precedence for it, though I can't remember which command > it was). This is not an interface we'll want to keep long term. What about blockdev-snapshot-sync/snapshot_blkdev? Paolo