[libvirt] [PATCH] storage: minor fix in luks encrypted volume creation
This patch fixes the case when creating a luks encrypted volume via an xml file without 'secret' element. libvirtd was receiving SIGSEGV, now proper error is reported for the missing element. (see bz 1468422) Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com> --- src/storage/storage_util.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 554fc757ed..18414ddb89 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1277,6 +1277,13 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, return NULL; } + if (!enc->secrets) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("A single <secret type='passphrase'...> " + "element is expected in encryption description")); + return NULL; + } + conn = virGetConnectSecret(); if (!conn) return NULL; -- 2.15.0
On Wed, 2018-06-06 at 16:54 +0200, Katerina Koukiou wrote:
This patch fixes the case when creating a luks encrypted volume via an xml file without 'secret' element. libvirtd was receiving SIGSEGV, now proper error is reported for the missing element. (see bz 1468422)
Something like Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1468422 would be better, as it disambiguates which specifica Bugzilla instance you're talking about and allows users to jump straight to the bug report. [...]
+ if (!enc->secrets) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("A single <secret type='passphrase'...> " + "element is expected in encryption description")); + return NULL; + }
I'm not very familiar with this area of libvirt, but I would probably error out when enc->nsecrets != 1 instead. Reporting this as VIR_ERR_INTERNAL_ERROR also doesn't look quite right; VIR_ERR_INVALID_ARG would certainly be more appropriate for something that happened because the user provided less information than expected. Hopefully someone more authoritative will chime in: to be completely honest, I really just wanted to comment on the commit message bit :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jun 06, 2018 at 05:30:15PM +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 16:54 +0200, Katerina Koukiou wrote:
This patch fixes the case when creating a luks encrypted volume via an xml file without 'secret' element. libvirtd was receiving SIGSEGV, now proper error is reported for the missing element. (see bz 1468422)
Something like
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1468422
Actually, the 'Resolves: ' part is pointless. Jano
would be better, as it disambiguates which specifica Bugzilla instance you're talking about and allows users to jump straight to the bug report.
On Wed, 2018-06-06 at 17:35 +0200, Ján Tomko wrote:
On Wed, Jun 06, 2018 at 05:30:15PM +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 16:54 +0200, Katerina Koukiou wrote: [....]
(see bz 1468422)
Something like
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1468422
Actually, the 'Resolves: ' part is pointless.
I happen to like it because it falls in neatly with the other pseudo-headers we use in commit messages, but I will indeed not lose any sleep if she ends up omitting it :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jun 06, 2018 at 05:41:58PM +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 17:35 +0200, Ján Tomko wrote:
On Wed, Jun 06, 2018 at 05:30:15PM +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 16:54 +0200, Katerina Koukiou wrote: [....]
(see bz 1468422)
Something like
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1468422
Actually, the 'Resolves: ' part is pointless.
I happen to like it because it falls in neatly with the other pseudo-headers we use in commit messages, but I will indeed not lose any sleep if she ends up omitting it :)
If for some reason the software displaying the commit message does not make the link clickable, the 'Resolves:' part hurts usability. Without it, I can triple-click to select the link and paste it into my browser. Jano
On Wed, 2018-06-06 at 18:19 +0200, Ján Tomko wrote:
On Wed, Jun 06, 2018 at 05:41:58PM +0200, Andrea Bolognani wrote:
On Wed, 2018-06-06 at 17:35 +0200, Ján Tomko wrote:
On Wed, Jun 06, 2018 at 05:30:15PM +0200, Andrea Bolognani wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1468422
Actually, the 'Resolves: ' part is pointless.
I happen to like it because it falls in neatly with the other pseudo-headers we use in commit messages, but I will indeed not lose any sleep if she ends up omitting it :)
If for some reason the software displaying the commit message does not make the link clickable, the 'Resolves:' part hurts usability.
Without it, I can triple-click to select the link and paste it into my browser.
I would argue what's hurting usability is software not making URLs clickable in the first place. -- Andrea Bolognani / Red Hat / Virtualization
s/minor fix/fix crash/ in the title On Wed, Jun 06, 2018 at 04:54:20PM +0200, Katerina Koukiou wrote:
This patch fixes the case when creating a luks encrypted volume
s/This patch fixes/Fix/
via an xml file without 'secret' element. libvirtd was receiving SIGSEGV, now proper error is reported for the missing element.
(see bz 1468422)
Please use a clickable bugzilla link on a separated line: https://bugzilla.redhat.com/show_bug.cgi?id=1468422
Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com> --- src/storage/storage_util.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 554fc757ed..18414ddb89 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1277,6 +1277,13 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, return NULL; }
+ if (!enc->secrets) {
You can use enc->secrets != 1, to match the error message. But the > 1 cause will be caught anyway later in storageBackendCreateQemuImgCheckEncryption which cannot be simply called before here because the code is overly complicated. With the bug link fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, 2018-06-06 at 17:30 +0200, Ján Tomko wrote: [...]
+ if (!enc->secrets) {
You can use enc->secrets != 1, to match the error message.
But the > 1 cause will be caught anyway later in storageBackendCreateQemuImgCheckEncryption which cannot be simply called before here because the code is overly complicated.
I'd rather the check be here, where we raise the error message, instead of relying on the fact that the other function will never change in a way that will cause it not to check for >1. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Katerina Koukiou