1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change

Michal Privoznik posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f9fed3738baac0a3bc2e0e16fadabbef670089ad.1513175405.git.mprivozn@redhat.com
src/qemu/qemu_domain.c | 8 ++++++++
1 file changed, 8 insertions(+)
[libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by Michal Privoznik 6 years, 4 months ago 
Since we have user aliases it may happen that users want to
change it using 'update-device'. Instead of ignoring it silently,
error out loudly.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 347fc0742..00d6c41c9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
         return false;
     }
 
+    if (disk->info.alias &&
+        STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                       _("cannot modify field '%s' of the disk"),
+                       "alias");
+        return false;
+    }
+
     CHECK_EQ(info.bootIndex, "boot order", true);
     CHECK_EQ(rawio, "rawio", true);
     CHECK_EQ(sgio, "sgio", true);
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by Michal Privoznik 6 years, 3 months ago 
On 12/13/2017 03:30 PM, Michal Privoznik wrote:
> Since we have user aliases it may happen that users want to
> change it using 'update-device'. Instead of ignoring it silently,
> error out loudly.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Ping.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by John Ferlan 6 years, 3 months ago 

On 12/13/2017 09:30 AM, Michal Privoznik wrote:
> Since we have user aliases it may happen that users want to
> change it using 'update-device'. Instead of ignoring it silently,
> error out loudly.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

So IIUC this would be adding a check that the libvirt supplied alias
matches, true? But your goal is to check if a "ua-" prefixed alias was
being used - so should the check use of the prefix?

There's probably a few more info. fields that could be considered - any
thoughts on those? Perhaps fairly difficult to keep up with the various
_virDomainDiskDef and friends alterations for what this function is
indicating...

John

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 347fc0742..00d6c41c9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>          return false;
>      }
>  
> +    if (disk->info.alias &&
> +        STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                       _("cannot modify field '%s' of the disk"),
> +                       "alias");
> +        return false;
> +    }
> +
>      CHECK_EQ(info.bootIndex, "boot order", true);
>      CHECK_EQ(rawio, "rawio", true);
>      CHECK_EQ(sgio, "sgio", true);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by Michal Privoznik 6 years, 3 months ago 
On 01/05/2018 01:15 AM, John Ferlan wrote:
> 
> 
> On 12/13/2017 09:30 AM, Michal Privoznik wrote:
>> Since we have user aliases it may happen that users want to
>> change it using 'update-device'. Instead of ignoring it silently,
>> error out loudly.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
> 
> So IIUC this would be adding a check that the libvirt supplied alias
> matches, true? But your goal is to check if a "ua-" prefixed alias was
> being used - so should the check use of the prefix?

I don't think this is limited to user provided aliases only. For
instance, libvirt generates the following aliases for disks: scsi0-0-0,
ide0-1-0, fdc0-0-0.

Now, what should happen if an user provides new disk XML trying to
change nothing but 'scsi0-0-0' to say 'scsi0-0-1' (for live domain)? I
think it should be forbidden - we don't have way to change IDs in qemu,
do we? We certainly lack implementation to support it.

Therefore, I think we don't have to limit the check just for 'ua-' prefixes.

> 
> There's probably a few more info. fields that could be considered - any
> thoughts on those? Perhaps fairly difficult to keep up with the various
> _virDomainDiskDef and friends alterations for what this function is
> indicating...

Yeah, probably. I'm not sure. Maybe we will find some more.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by John Ferlan 6 years, 3 months ago 

On 01/05/2018 03:20 AM, Michal Privoznik wrote:
> On 01/05/2018 01:15 AM, John Ferlan wrote:
>>
>>
>> On 12/13/2017 09:30 AM, Michal Privoznik wrote:
>>> Since we have user aliases it may happen that users want to
>>> change it using 'update-device'. Instead of ignoring it silently,
>>> error out loudly.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>
>> So IIUC this would be adding a check that the libvirt supplied alias
>> matches, true? But your goal is to check if a "ua-" prefixed alias was
>> being used - so should the check use of the prefix?
> 
> I don't think this is limited to user provided aliases only. For
> instance, libvirt generates the following aliases for disks: scsi0-0-0,
> ide0-1-0, fdc0-0-0.
> 
> Now, what should happen if an user provides new disk XML trying to
> change nothing but 'scsi0-0-0' to say 'scsi0-0-1' (for live domain)? I
> think it should be forbidden - we don't have way to change IDs in qemu,
> do we? We certainly lack implementation to support it.
> 
> Therefore, I think we don't have to limit the check just for 'ua-' prefixes.
> 

OK - fair enough...  Perhaps the commit message is what caught my
attention/focus to ua- in particular.  Maybe the extra explanation here
can be worked in.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

>>
>> There's probably a few more info. fields that could be considered - any
>> thoughts on those? Perhaps fairly difficult to keep up with the various
>> _virDomainDiskDef and friends alterations for what this function is
>> indicating...
> 
> Yeah, probably. I'm not sure. Maybe we will find some more.
> 
> Michal
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by Marc Hartmayer 6 years, 3 months ago 
On Wed, Dec 13, 2017 at 03:30 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
> Since we have user aliases it may happen that users want to
> change it using 'update-device'. Instead of ignoring it silently,
> error out loudly.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 347fc0742..00d6c41c9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>          return false;
>      }
>
> +    if (disk->info.alias &&
> +        STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                       _("cannot modify field '%s' of the disk"),
> +                       "alias");
> +        return false;
> +    }
> +
>      CHECK_EQ(info.bootIndex, "boot order", true);
>      CHECK_EQ(rawio, "rawio", true);
>      CHECK_EQ(sgio, "sgio", true);
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>

I'm not sure if our current approach is the way to go… Couldn’t we just
invert the logic for qemuDomainDiskChangeSupported in that way that it
returns by default false and only for cases where we know that it’s
supported to change we return true? Not sure if this proposed approach
is feasible.

--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by Michal Privoznik 6 years, 3 months ago 
On 01/05/2018 01:16 PM, Marc Hartmayer wrote:
> On Wed, Dec 13, 2017 at 03:30 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
>> Since we have user aliases it may happen that users want to
>> change it using 'update-device'. Instead of ignoring it silently,
>> error out loudly.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 347fc0742..00d6c41c9 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>>          return false;
>>      }
>>
>> +    if (disk->info.alias &&
>> +        STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
>> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> +                       _("cannot modify field '%s' of the disk"),
>> +                       "alias");
>> +        return false;
>> +    }
>> +
>>      CHECK_EQ(info.bootIndex, "boot order", true);
>>      CHECK_EQ(rawio, "rawio", true);
>>      CHECK_EQ(sgio, "sgio", true);
>> --
>> 2.13.6
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 
> I'm not sure if our current approach is the way to go… Couldn’t we just
> invert the logic for qemuDomainDiskChangeSupported in that way that it
> returns by default false and only for cases where we know that it’s
> supported to change we return true? Not sure if this proposed approach
> is feasible.

Maybe. If you want to work on it, be my guest. It looks feasible to me.
Unfortunately, I'm buried under some other work right now.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuDomainDiskChangeSupported: Forbid alias change
Posted by Marc Hartmayer 6 years, 3 months ago 
On Fri, Jan 05, 2018 at 02:24 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
> On 01/05/2018 01:16 PM, Marc Hartmayer wrote:
>> On Wed, Dec 13, 2017 at 03:30 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
>>> Since we have user aliases it may happen that users want to
>>> change it using 'update-device'. Instead of ignoring it silently,
>>> error out loudly.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 347fc0742..00d6c41c9 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -6762,6 +6762,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>>>          return false;
>>>      }
>>>
>>> +    if (disk->info.alias &&
>>> +        STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
>>> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> +                       _("cannot modify field '%s' of the disk"),
>>> +                       "alias");
>>> +        return false;
>>> +    }
>>> +
>>>      CHECK_EQ(info.bootIndex, "boot order", true);
>>>      CHECK_EQ(rawio, "rawio", true);
>>>      CHECK_EQ(sgio, "sgio", true);
>>> --
>>> 2.13.6
>>>
>>> --
>>> libvir-list mailing list
>>> libvir-list@redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>>>
>>
>> I'm not sure if our current approach is the way to go… Couldn’t we just
>> invert the logic for qemuDomainDiskChangeSupported in that way that it
>> returns by default false and only for cases where we know that it’s
>> supported to change we return true? Not sure if this proposed approach
>> is feasible.
>
> Maybe. If you want to work on it, be my guest. It looks feasible to
> me.

Currently, I don't have time for this as it’s not that important.

> Unfortunately, I'm buried under some other work right now.
>
> Michal
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.