On Tue 20 Jun 2017 02:02:06 PM CEST, Daniel P. Berrange wrote: >> > + if (encryptfmt) { >> > + buf = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT); >> > + if (buf != NULL) { >> > + g_free(buf); >> >> If you use qemu_opt_get() instead then you don't need "buf" at all, >> do you? > > IIRC, we needed to delete the option from opts, otherwise something > will later complain that there are opts that are not consumed.
I don't see how, since once you reached this point it means that you cannot create the image because the options are already wrong. Anyway, there's a more important problem with this patch that I just noticed: in this scenario you're freeing 'buf' twice, first in the snip I quoted above, and then at the end of qcow2_create(). qcow_create() from qcow.c is safe, but perhaps it's clearer if you put the declaration of the 'buf' variable inside the 'if (encryptfmt)' block. That assuming that you really need to use qemu_opt_get_del() there. With the double-free bug fixed, I don't see any difference when using qemu_opt_get_del() vs qemu_opt_get(). Berto