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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.