Re: [virt-tools-list] [PATCH virt-viewer 01/11] ovirt-foreign-menu: Rework states logic
Hi Eduardo,
On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> Use switch/case instead of lots of conditional blocks
Yes, it is more readable
>
> Signed-off-by: Eduardo Lima (Etrunko)
> ---
> src/ovirt-foreign-menu.c | 76 +++--
> ---
> 1 file changed, 36 insertions(+), 40 deletions(-)
>
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 33ff4f1..b0b8fec 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -312,51 +312,47 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu
> *menu,
> g_return_if_fail(current_state >= STATE_0);
> g_return_if_fail(current_state < STATE_ISOS);
>
> -current_state++;
my preference is to keep the increment outside the switch statement
> -
> -if (current_state == STATE_API) {
> -if (menu->priv->api == NULL) {
> -ovirt_foreign_menu_fetch_api_async(menu);
> -} else {
> -current_state++;
> +switch (++current_state) {
Actually the increment is not needed at all thanks to your changes, imo
switch(current_state + 1) would be more readable
> +case STATE_API: {
'case' should have the same indentation as its 'switch'
Remove extra {}, no need to have the null check in the extra block (applies to
all cases)
Pavel
> +if (menu->priv->api == NULL) {
> +ovirt_foreign_menu_fetch_api_async(menu);
> +break;
> +}
> }
> -}
> -
> -if (current_state == STATE_VM) {
> -if (menu->priv->vm == NULL) {
> -ovirt_foreign_menu_fetch_vm_async(menu);
> -} else {
> -current_state++;
> +case STATE_VM: {
> +if (menu->priv->vm == NULL) {
> +ovirt_foreign_menu_fetch_vm_async(menu);
> +break;
> +}
> }
> -}
> -
> -if (current_state == STATE_STORAGE_DOMAIN) {
> -if (menu->priv->files == NULL) {
> -ovirt_foreign_menu_fetch_storage_domain_async(menu);
> -} else {
> -current_state++;
> +case STATE_STORAGE_DOMAIN: {
> +if (menu->priv->files == NULL) {
> +ovirt_foreign_menu_fetch_storage_domain_async(menu);
> +break;
> +}
> }
> -}
> -
> -if (current_state == STATE_VM_CDROM) {
> -if (menu->priv->cdrom == NULL) {
> -ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
> -} else {
> -current_state++;
> +case STATE_VM_CDROM: {
> +if (menu->priv->cdrom == NULL) {
> +ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
> +break;
> +}
> }
> -}
> -
> -if (current_state == STATE_CDROM_FILE) {
> -ovirt_foreign_menu_refresh_cdrom_file_async(menu);
> -}
> -
> -if (current_state == STATE_ISOS) {
> -g_warn_if_fail(menu->priv->api != NULL);
> -g_warn_if_fail(menu->priv->vm != NULL);
> -g_warn_if_fail(menu->priv->files != NULL);
> -g_warn_if_fail(menu->priv->cdrom != NULL);
> +case STATE_CDROM_FILE: {
> +ovirt_foreign_menu_refresh_cdrom_file_async(menu);
> +break;
> +}
> +case STATE_ISOS: {
> +g_warn_if_fail(menu->priv->api != NULL);
> +g_warn_if_fail(menu->priv->vm != NULL);
> +g_warn_if_fail(menu->priv->files != NULL);
> +g_warn_if_fail(menu->priv->cdrom != NULL);
>
> -ovirt_foreign_menu_refresh_iso_list(menu);
> +ovirt_foreign_menu_refresh_iso_list(menu);
> +break;
> +}
> +default: {
> +g_warn_if_reached();
> +}
> }
> }
>
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer 03/11] ovirt-foreign-menu: Remove timer used to refresh iso list
On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> With the new ISO dialog, the user triggers the refresh manually.
This should come after the dialog is introduced>
> Signed-off-by: Eduardo Lima (Etrunko)
> ---
> src/ovirt-foreign-menu.c | 23 +++
> 1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 889e7bc..b071e27 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -46,7 +46,7 @@ static void
> ovirt_foreign_menu_fetch_vm_async(OvirtForeignMenu *menu);
> static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu
> *menu);
> static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu);
> static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu
> *menu);
> -static gboolean ovirt_foreign_menu_refresh_iso_list(gpointer user_data);
> +static void ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu);
>
> G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
>
> @@ -313,7 +313,7 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
> g_warn_if_fail(menu->priv->files != NULL);
> g_warn_if_fail(menu->priv->cdrom != NULL);
>
> -ovirt_foreign_menu_refresh_iso_list(menu);
> +ovirt_foreign_menu_fetch_iso_list_async(menu);
> break;
> }
> default: {
> @@ -756,8 +756,6 @@ static void iso_list_fetched_cb(GObject *source_object,
> files =
> g_hash_table_get_values(ovirt_collection_get_resources(collection));
> ovirt_foreign_menu_set_files(OVIRT_FOREIGN_MENU(user_data), files);
> g_list_free(files);
> -
> -g_timeout_add_seconds(300, ovirt_foreign_menu_refresh_iso_list,
> user_data);
> }
>
>
> @@ -767,27 +765,12 @@ static void
> ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu)
> return;
> }
>
> +g_debug("Refreshing foreign menu iso list");
> ovirt_collection_fetch_async(menu->priv->files, menu->priv->proxy,
> NULL, iso_list_fetched_cb, menu);
> }
>
>
> -static gboolean ovirt_foreign_menu_refresh_iso_list(gpointer user_data)
> -{
> -OvirtForeignMenu *menu;
> -
> -g_debug("Refreshing foreign menu iso list");
> -menu = OVIRT_FOREIGN_MENU(user_data);
> -ovirt_foreign_menu_fetch_iso_list_async(menu);
> -
> -/* ovirt_foreign_menu_fetch_iso_list_async() will schedule a new call to
> - * that function through iso_list_fetched_cb() when it has finished
> - * fetching the iso list
> - */
> -return G_SOURCE_REMOVE;
> -}
> -
> -
> OvirtForeignMenu *ovirt_foreign_menu_new_from_file(VirtViewerFile *file)
> {
> OvirtProxy *proxy = NULL;
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer 03/11] ovirt-foreign-menu: Remove timer used to refresh iso list
On 07/18/2016 09:14 AM, Pavel Grunt wrote: > On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote: >> With the new ISO dialog, the user triggers the refresh manually. > > This should come after the dialog is introduced> Well, I have no plans in merging any of these patches independently, they are only split so that it makes more sense to understand the whole series. -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etru...@redhat.com ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer 01/11] ovirt-foreign-menu: Rework states logic
On 07/18/2016 06:15 AM, Pavel Grunt wrote:
> Hi Eduardo,
>
> On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
>> Use switch/case instead of lots of conditional blocks
> Yes, it is more readable
>>
>> Signed-off-by: Eduardo Lima (Etrunko)
>> ---
>> src/ovirt-foreign-menu.c | 76 +++--
>> ---
>> 1 file changed, 36 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
>> index 33ff4f1..b0b8fec 100644
>> --- a/src/ovirt-foreign-menu.c
>> +++ b/src/ovirt-foreign-menu.c
>> @@ -312,51 +312,47 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu
>> *menu,
>> g_return_if_fail(current_state >= STATE_0);
>> g_return_if_fail(current_state < STATE_ISOS);
>>
>> -current_state++;
> my preference is to keep the increment outside the switch statement
>
>> -
>> -if (current_state == STATE_API) {
>> -if (menu->priv->api == NULL) {
>> -ovirt_foreign_menu_fetch_api_async(menu);
>> -} else {
>> -current_state++;
>> +switch (++current_state) {
> Actually the increment is not needed at all thanks to your changes, imo
> switch(current_state + 1) would be more readable
Alright, I don't mind at all.
>> +case STATE_API: {
> 'case' should have the same indentation as its 'switch'
>
> Remove extra {}, no need to have the null check in the extra block (applies to
> all cases)
>
I don't think so, the if checks are necessary for the initialization
process, when we have everything initalized, it will fall straight to
the STATE_ISOS case. Or maybe you are talking about something else and I
misunderstood?
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
[virt-tools-list] [PATCH virt-viewer v2 01/11] ovirt-foreign-menu: Rework states logic
Use switch/case instead of lots of conditional blocks
Signed-off-by: Eduardo Lima (Etrunko)
---
src/ovirt-foreign-menu.c | 41 +++--
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
index 33ff4f1..80e40a1 100644
--- a/src/ovirt-foreign-menu.c
+++ b/src/ovirt-foreign-menu.c
@@ -312,51 +312,40 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
g_return_if_fail(current_state >= STATE_0);
g_return_if_fail(current_state < STATE_ISOS);
-current_state++;
-
-if (current_state == STATE_API) {
+switch (++current_state) {
+case STATE_API:
if (menu->priv->api == NULL) {
ovirt_foreign_menu_fetch_api_async(menu);
-} else {
-current_state++;
+break;
}
-}
-
-if (current_state == STATE_VM) {
+case STATE_VM:
if (menu->priv->vm == NULL) {
ovirt_foreign_menu_fetch_vm_async(menu);
-} else {
-current_state++;
+break;
}
-}
-
-if (current_state == STATE_STORAGE_DOMAIN) {
+case STATE_STORAGE_DOMAIN:
if (menu->priv->files == NULL) {
ovirt_foreign_menu_fetch_storage_domain_async(menu);
-} else {
-current_state++;
+break;
}
-}
-
-if (current_state == STATE_VM_CDROM) {
+case STATE_VM_CDROM:
if (menu->priv->cdrom == NULL) {
ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
-} else {
-current_state++;
+break;
}
-}
-
-if (current_state == STATE_CDROM_FILE) {
+case STATE_CDROM_FILE:
ovirt_foreign_menu_refresh_cdrom_file_async(menu);
-}
-
-if (current_state == STATE_ISOS) {
+break;
+case STATE_ISOS:
g_warn_if_fail(menu->priv->api != NULL);
g_warn_if_fail(menu->priv->vm != NULL);
g_warn_if_fail(menu->priv->files != NULL);
g_warn_if_fail(menu->priv->cdrom != NULL);
ovirt_foreign_menu_refresh_iso_list(menu);
+break;
+default:
+g_warn_if_reached();
}
}
--
2.7.4
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer 01/11] ovirt-foreign-menu: Rework states logic
On Mon, 2016-07-18 at 10:22 -0300, Eduardo Lima (Etrunko) wrote:
> On 07/18/2016 06:15 AM, Pavel Grunt wrote:
> >
> > Hi Eduardo,
> >
> > On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> > >
> > > Use switch/case instead of lots of conditional blocks
> > Yes, it is more readable
> > >
> > >
> > > Signed-off-by: Eduardo Lima (Etrunko)
> > > ---
> > > src/ovirt-foreign-menu.c | 76 +++--
> > >
> > > ---
> > > 1 file changed, 36 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> > > index 33ff4f1..b0b8fec 100644
> > > --- a/src/ovirt-foreign-menu.c
> > > +++ b/src/ovirt-foreign-menu.c
> > > @@ -312,51 +312,47 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu
> > > *menu,
> > > g_return_if_fail(current_state >= STATE_0);
> > > g_return_if_fail(current_state < STATE_ISOS);
> > >
> > > -current_state++;
> > my preference is to keep the increment outside the switch statement
> >
> > >
> > > -
> > > -if (current_state == STATE_API) {
> > > -if (menu->priv->api == NULL) {
> > > -ovirt_foreign_menu_fetch_api_async(menu);
> > > -} else {
> > > -current_state++;
> > > +switch (++current_state) {
> > Actually the increment is not needed at all thanks to your changes, imo
> > switch(current_state + 1) would be more readable
>
> Alright, I don't mind at all.
>
> >
> > >
> > > +case STATE_API: {
> > 'case' should have the same indentation as its 'switch'
> >
> > Remove extra {}, no need to have the null check in the extra block (applies
> > to
> > all cases)
> >
>
> I don't think so, the if checks are necessary for the initialization
> process, when we have everything initalized, it will fall straight to
> the STATE_ISOS case. Or maybe you are talking about something else and I
> misunderstood?
>
the {} are not needed, but it is a minor:
...
case STATE_API:
if (menu->priv->api == NULL) {
ovirt_foreign_menu_fetch_api_async(menu);
break;
}
case STATE_VM:
if (menu->priv->vm == NULL) {
ovirt_foreign_menu_fetch_vm_async(menu);
break;
}
...
>
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer 01/11] ovirt-foreign-menu: Rework states logic
On 07/18/2016 11:58 AM, Pavel Grunt wrote:
> On Mon, 2016-07-18 at 10:22 -0300, Eduardo Lima (Etrunko) wrote:
>> On 07/18/2016 06:15 AM, Pavel Grunt wrote:
>>>
>>> Hi Eduardo,
>>>
>>> On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
Use switch/case instead of lots of conditional blocks
>>> Yes, it is more readable
Signed-off-by: Eduardo Lima (Etrunko)
---
src/ovirt-foreign-menu.c | 76 +++--
---
1 file changed, 36 insertions(+), 40 deletions(-)
diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
index 33ff4f1..b0b8fec 100644
--- a/src/ovirt-foreign-menu.c
+++ b/src/ovirt-foreign-menu.c
@@ -312,51 +312,47 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu
*menu,
g_return_if_fail(current_state >= STATE_0);
g_return_if_fail(current_state < STATE_ISOS);
-current_state++;
>>> my preference is to keep the increment outside the switch statement
>>>
-
-if (current_state == STATE_API) {
-if (menu->priv->api == NULL) {
-ovirt_foreign_menu_fetch_api_async(menu);
-} else {
-current_state++;
+switch (++current_state) {
>>> Actually the increment is not needed at all thanks to your changes, imo
>>> switch(current_state + 1) would be more readable
>>
>> Alright, I don't mind at all.
>>
>>>
+case STATE_API: {
>>> 'case' should have the same indentation as its 'switch'
>>>
>>> Remove extra {}, no need to have the null check in the extra block (applies
>>> to
>>> all cases)
>>>
>>
>> I don't think so, the if checks are necessary for the initialization
>> process, when we have everything initalized, it will fall straight to
>> the STATE_ISOS case. Or maybe you are talking about something else and I
>> misunderstood?
>>
>
> the {} are not needed, but it is a minor:
The break statements are put inside of the conditional block on purpose.
If the NULL check passes, the initialization is done asynchronously and
that function will be responsible for calling back with the next step.
If it fails, it means that it was already initialized, so we move to the
next initialization, one at a time, until everything is set up and it
will fall straight to the STATE_ISOS.
> ...
> case STATE_API:
> if (menu->priv->api == NULL) {
> ovirt_foreign_menu_fetch_api_async(menu);
> break;
> }
> case STATE_VM:
> if (menu->priv->vm == NULL) {
> ovirt_foreign_menu_fetch_vm_async(menu);
> break;
> }
> ...
>
>>
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer 01/11] ovirt-foreign-menu: Rework states logic
On Mon, 2016-07-18 at 12:18 -0300, Eduardo Lima (Etrunko) wrote:
> On 07/18/2016 11:58 AM, Pavel Grunt wrote:
> >
> > On Mon, 2016-07-18 at 10:22 -0300, Eduardo Lima (Etrunko) wrote:
> > >
> > > On 07/18/2016 06:15 AM, Pavel Grunt wrote:
> > > >
> > > >
> > > > Hi Eduardo,
> > > >
> > > > On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> > > > >
> > > > >
> > > > > Use switch/case instead of lots of conditional blocks
> > > > Yes, it is more readable
> > > > >
> > > > >
> > > > >
> > > > > Signed-off-by: Eduardo Lima (Etrunko)
> > > > > ---
> > > > > src/ovirt-foreign-menu.c | 76 +++--
> > > > >
> > > > >
> > > > > ---
> > > > > 1 file changed, 36 insertions(+), 40 deletions(-)
> > > > >
> > > > > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> > > > > index 33ff4f1..b0b8fec 100644
> > > > > --- a/src/ovirt-foreign-menu.c
> > > > > +++ b/src/ovirt-foreign-menu.c
> > > > > @@ -312,51 +312,47 @@
> > > > > ovirt_foreign_menu_next_async_step(OvirtForeignMenu
> > > > > *menu,
> > > > > g_return_if_fail(current_state >= STATE_0);
> > > > > g_return_if_fail(current_state < STATE_ISOS);
> > > > >
> > > > > -current_state++;
> > > > my preference is to keep the increment outside the switch statement
> > > >
> > > > >
> > > > >
> > > > > -
> > > > > -if (current_state == STATE_API) {
> > > > > -if (menu->priv->api == NULL) {
> > > > > -ovirt_foreign_menu_fetch_api_async(menu);
> > > > > -} else {
> > > > > -current_state++;
> > > > > +switch (++current_state) {
> > > > Actually the increment is not needed at all thanks to your changes, imo
> > > > switch(current_state + 1) would be more readable
> > >
> > > Alright, I don't mind at all.
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > +case STATE_API: {
> > > > 'case' should have the same indentation as its 'switch'
> > > >
> > > > Remove extra {}, no need to have the null check in the extra block
> > > > (applies
> > > > to
> > > > all cases)
> > > >
> > >
> > > I don't think so, the if checks are necessary for the initialization
> > > process, when we have everything initalized, it will fall straight to
> > > the STATE_ISOS case. Or maybe you are talking about something else and I
> > > misunderstood?
> > >
> >
> > the {} are not needed, but it is a minor:
>
> The break statements are put inside of the conditional block on purpose.
yes, of course. I am not talking about break statements
> If the NULL check passes, the initialization is done asynchronously and
> that function will be responsible for calling back with the next step.
> If it fails, it means that it was already initialized, so we move to the
> next initialization, one at a time, until everything is set up and it
> will fall straight to the STATE_ISOS.
>
> >
> > ...
> > case STATE_API:
> > if (menu->priv->api == NULL) {
> > ovirt_foreign_menu_fetch_api_async(menu);
> > break;
> > }
> > case STATE_VM:
> > if (menu->priv->vm == NULL) {
> > ovirt_foreign_menu_fetch_vm_async(menu);
> > break;
> > }
> > ...
> >
I tried to show it ^, but to be more explicit:
instead of
case A: {
if () {
break
}
}
just
case A:
if () {
break
}
But you have already changed it for v2 :)
Pavel
> > >
> > >
>
>
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2 01/11] ovirt-foreign-menu: Rework states logic
On Mon, 2016-07-18 at 10:25 -0300, Eduardo Lima (Etrunko) wrote:
> Use switch/case instead of lots of conditional blocks
>
> Signed-off-by: Eduardo Lima (Etrunko)
Acked-by: Pavel Grunt
> ---
> src/ovirt-foreign-menu.c | 41 +++--
> 1 file changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 33ff4f1..80e40a1 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -312,51 +312,40 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu
> *menu,
> g_return_if_fail(current_state >= STATE_0);
> g_return_if_fail(current_state < STATE_ISOS);
>
> -current_state++;
> -
> -if (current_state == STATE_API) {
> +switch (++current_state) {
> +case STATE_API:
> if (menu->priv->api == NULL) {
> ovirt_foreign_menu_fetch_api_async(menu);
> -} else {
> -current_state++;
> +break;
> }
> -}
> -
> -if (current_state == STATE_VM) {
> +case STATE_VM:
> if (menu->priv->vm == NULL) {
> ovirt_foreign_menu_fetch_vm_async(menu);
> -} else {
> -current_state++;
> +break;
> }
> -}
> -
> -if (current_state == STATE_STORAGE_DOMAIN) {
> +case STATE_STORAGE_DOMAIN:
> if (menu->priv->files == NULL) {
> ovirt_foreign_menu_fetch_storage_domain_async(menu);
> -} else {
> -current_state++;
> +break;
> }
> -}
> -
> -if (current_state == STATE_VM_CDROM) {
> +case STATE_VM_CDROM:
> if (menu->priv->cdrom == NULL) {
> ovirt_foreign_menu_fetch_vm_cdrom_async(menu);
> -} else {
> -current_state++;
> +break;
> }
> -}
> -
> -if (current_state == STATE_CDROM_FILE) {
> +case STATE_CDROM_FILE:
> ovirt_foreign_menu_refresh_cdrom_file_async(menu);
> -}
> -
> -if (current_state == STATE_ISOS) {
> +break;
> +case STATE_ISOS:
> g_warn_if_fail(menu->priv->api != NULL);
> g_warn_if_fail(menu->priv->vm != NULL);
> g_warn_if_fail(menu->priv->files != NULL);
> g_warn_if_fail(menu->priv->cdrom != NULL);
>
> ovirt_foreign_menu_refresh_iso_list(menu);
> +break;
> +default:
> +g_warn_if_reached();
> }
> }
>
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH 0/2] virtinst: Pass SMBios information to guest
On 07/14/2016 10:06 AM, Charles Arnold wrote:
> This patchset introduces a command line option to pass smbios information to
> the guest. It adds an smbios mode sub-element to the guest's os element. The
> mode attribute must be specified and is either "emulate" (let the hypervisor
> generate all values), "host" (copy all of Block 0 and Block 1, except for the
> UUID, from the host's SMBIOS values), or "sysinfo" (use the values in the
> sysinfo element). If not specified, the hypervisor default is used.
>
> If sysinfo is specified in the smbios mode field then an additional sysinfo
> element is added to the guest defining the SMBios information. The sysinfo
> element has a mandatory attribute type of "smbios". The smbios information
> comes as three sub-types, "bios", "system", and "baseBoard" where each of
> these types contains sub-elements .
>
> Incorrectly supplied entries for the bios, system or baseBoard blocks will be
> ignored without error. Other than uuid validation and date format checking,
> all
> values are passed as strings to the hypervisor driver.
>
> Examples:
> --sysinfo emulate
> --sysinfo host
> --sysinfo type=0,vendor=Vendor_Inc.,version=1.2.3-abc,...
> --sysinfo type=1,manufacturer=System_Corp.,product=codename,...
> --sysinfo type=2,manufacturer=Baseboard_Corp.,product=codename,...
>
>
Thanks for reworking the patches. I see now that you were trying to mirror the
qemu command line -smbios format. Were you just following that as an example,
or do you have a particularly compelling reason for it?
virt-install cli naming should map to libvirt values as much as possible,
(minus the '' naming which is completely redundant) maybe some day
libvirt will support a non-qemu , or a non-smbios , or the
user doesn't know that the syntax is for matching with qemu, etc, so we
shouldn't be tied to qemu syntax.
I pushed some patches upstream that make XML properties like this work
correctly when setting the value:
bios_vendor = XMLProperty("./bios/entry[@name='vendor']")
So I think SYSInfo should be altered to explicitly track bios_* , system_*,
and baseBoard_* properties, and the --sysinfo command line option should have
bios.*, system.*, and baseboard.* properties to match. That way '--sysinfo
help' introspection works as expected too.
_If_ providing an option for the qemu syntax is important to you we can still
support it, but it needs to be explicit. Maybe something like --sysinfo
qemu-cli='type=1,Manufacturer=X,...'. But it should be an additive thing, not
the main way to interact
Thanks,
Cole
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-install] Support for capturing guest serial console to a log file?
On 07/15/2016 10:58 AM, Kashyap Chamarthy wrote: > Currently, one could do a `virsh edit vm1`, to redirect guest serial > console output to a log file: > > > > > > > > > > > But that requires cumbersome manaul editing of the guest XML. Is it > possible yet via virt-install? > > Perhaps something like: > > virt-install \ > --serial pty, log,file=/tmp/f24-serial.log \ > --console pty, log,file=/tmp/f24-serial.log \ > [...] > > The said feature was introduced in libvirt via these commits (by Dan > Berrange): > > - http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=06cb0cf -- qemu: > add support for logging chardev output to a file > - http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=00ce10c -- > conf: allow use of a logfile with chardev backends > Nope, released virt-install doesn't support it, but I just fixed it upstream: commit 322d21251630d16ab6664477c10d7442d9f222e7 Author: Cole Robinson Date: Mon Jul 18 15:03:06 2016 -0400 cli: Add --serial log.file= and log.append= And for other character devices --console, --parallel, --channel Thanks, Cole ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH 0/2] virtinst: Pass SMBios information to guest
>>> On 7/18/2016 at 12:54 PM, Cole Robinson wrote:
> On 07/14/2016 10:06 AM, Charles Arnold wrote:
>> This patchset introduces a command line option to pass smbios information to
>> the guest. It adds an smbios mode sub-element to the guest's os element. The
>> mode attribute must be specified and is either "emulate" (let the hypervisor
>> generate all values), "host" (copy all of Block 0 and Block 1, except for
> the
>> UUID, from the host's SMBIOS values), or "sysinfo" (use the values in the
>> sysinfo element). If not specified, the hypervisor default is used.
>>
>> If sysinfo is specified in the smbios mode field then an additional sysinfo
>> element is added to the guest defining the SMBios information. The sysinfo
>> element has a mandatory attribute type of "smbios". The smbios information
>> comes as three sub-types, "bios", "system", and "baseBoard" where each of
>> these types contains sub-elements .
>>
>> Incorrectly supplied entries for the bios, system or baseBoard blocks will
> be
>> ignored without error. Other than uuid validation and date format checking,
> all
>> values are passed as strings to the hypervisor driver.
>>
>> Examples:
>> --sysinfo emulate
>> --sysinfo host
>> --sysinfo type=0,vendor=Vendor_Inc.,version=1.2.3-abc,...
>> --sysinfo type=1,manufacturer=System_Corp.,product=codename,...
>> --sysinfo type=2,manufacturer=Baseboard_Corp.,product=codename,...
>>
>>
>
> Thanks for reworking the patches. I see now that you were trying to mirror
> the
> qemu command line -smbios format. Were you just following that as an example,
> or do you have a particularly compelling reason for it?
Just following it as an example.
>
> virt-install cli naming should map to libvirt values as much as possible,
> (minus the '' naming which is completely redundant) maybe some day
> libvirt will support a non-qemu , or a non-smbios , or the
> user doesn't know that the syntax is for matching with qemu, etc, so we
> shouldn't be tied to qemu syntax.
Agreed. Libvirt's 'virsh sysinfo' command could someday be enhanced to do
what I'm trying to do with virt-install. If the work gets done it may also
follow
some entirely different syntax.
>
> I pushed some patches upstream that make XML properties like this work
> correctly when setting the value:
>
> bios_vendor = XMLProperty("./bios/entry[@name='vendor']")
>
> So I think SYSInfo should be altered to explicitly track bios_* , system_*,
> and baseBoard_* properties, and the --sysinfo command line option should have
> bios.*, system.*, and baseboard.* properties to match. That way '--sysinfo
> help' introspection works as expected too.
Ok, I'll work on that.
>
> _If_ providing an option for the qemu syntax is important to you we can
> still
> support it, but it needs to be explicit. Maybe something like --sysinfo
> qemu-cli='type=1,Manufacturer=X,...'. But it should be an additive thing, not
> the main way to interact
I'm not sold on any particular syntax for providing the information. Adding more
ways to provide the same thing may just unnecessarily complicate the usage.
Thanks for taking time to do the review,
Charles
>
> Thanks,
> Cole
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
