Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU

[Paolo Bonzini]

> 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