From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54828) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bor9j-0003lc-Vp for qemu-devel@nongnu.org; Tue, 27 Sep 2016 08:06:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bor9f-0003pw-Ln for qemu-devel@nongnu.org; Tue, 27 Sep 2016 08:06:38 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:37520) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bor9f-0003ph-Cm for qemu-devel@nongnu.org; Tue, 27 Sep 2016 08:06:35 -0400 Date: Tue, 27 Sep 2016 08:06:25 -0400 (EDT) From: Paolo Bonzini Message-ID: <1067795954.3059735.1474977985654.JavaMail.zimbra@redhat.com> In-Reply-To: References: <1474481984-10452-1-git-send-email-roysh@mellanox.com> <20160927102813.GE563@stefanha-x1.localdomain> <0537c2f5-39a7-2835-8f45-c7b43afcc50c@mellanox.com> <23477b74-e93a-923f-b83f-bf13fc255b3f@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: roysh@mellanox.com Cc: Stefan Hajnoczi , roy shterman , pl@kamp.de, qemu-devel@nongnu.org, ronniesahlberg@gmail.com > On 9/27/2016 2:52 PM, Paolo Bonzini wrote: > > On 27/09/2016 13:37, Roy Shterman wrote: > > > > > + iscsi_url = iscsi_parse_full_url(iscsi, > > > > > uri_string_unescape(filename, -1, NULL)); > > > > > if (iscsi_url == NULL) { > > > > > - error_setg(errp, "Failed to parse URL : %s", filename); > > > > > + error_setg(errp, "Failed to parse URL : %s", > > > > > uri_string_unescape(filename, -1, NULL)); > > > > > > > > uri_string_unescape() returns a newly allocated string. This is a > > > > memory leak! > > > > Is unescaping a bug fix? Please put it into a separate patch. > > > > > > because libvirt is parsing '?' char as %3F, I needed to parse to URI > > > with unescaping. > > > > This looks like a libvirt bug. But if libvirt learns to pass iser:// > > URIs, the unescape is not necessary, is it? > > right, but because libvirt inbox versions doesn't support > protocol_name=iser I decided to leave both options available. > The ?iser option and the iser:// option, to also have compatibility with > inbox Libvirt versions. No, please don't do that, especially because unescaping the URI is wrong: if somebody puts a %2F in the libvirt XML it should _not_ separate the IQN from the LUN number for example. Paolo