| Message ID | 20240613154406.1365469-13-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qga: clean up command source locations and conditionals | expand |
On Thu, 13 Jun 2024 18:44, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >The fsfreeze commands are already written to report an error if >vss_init() fails. Reporting a more specific error message is more >helpful than a generic "command is disabled" message, which cannot >beteween an admin config decision and lack of platform support. s/beteween/between > >Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >--- > qga/commands-win32.c | 18 +++--------------- > qga/main.c | 4 ++++ > 2 files changed, 7 insertions(+), 15 deletions(-) > >diff --git a/qga/commands-win32.c b/qga/commands-win32.c >index 2533e4c748..5866cc2e3c 100644 >--- a/qga/commands-win32.c >+++ b/qga/commands-win32.c >@@ -1203,7 +1203,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) > GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) > { > if (!vss_initialized()) { >- error_setg(errp, QERR_UNSUPPORTED); >+ error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); > return 0; Should this be return -1 by the way? > } > >@@ -1231,7 +1231,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints, > Error *local_err = NULL; > > if (!vss_initialized()) { >- error_setg(errp, QERR_UNSUPPORTED); >+ error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); > return 0; > } > >@@ -1266,7 +1266,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp) > int i; > > if (!vss_initialized()) { >- error_setg(errp, QERR_UNSUPPORTED); >+ error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); > return 0; > } > >@@ -1961,18 +1961,6 @@ done: > /* add unsupported commands to the list of blocked RPCs */ > GList *ga_command_init_blockedrpcs(GList *blockedrpcs) > { >- if (!vss_init(true)) { >- g_debug("vss_init failed, vss commands are going to be disabled"); >- const char *list[] = { >- "guest-get-fsinfo", "guest-fsfreeze-status", >- "guest-fsfreeze-freeze", "guest-fsfreeze-thaw", NULL}; >- char **p = (char **)list; >- >- while (*p) { >- blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++)); >- } >- } >- > return blockedrpcs; > } > >diff --git a/qga/main.c b/qga/main.c >index f4d5f15bb3..17b6ce18ac 100644 >--- a/qga/main.c >+++ b/qga/main.c >@@ -1395,6 +1395,10 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation) > " '%s': %s", config->state_dir, strerror(errno)); > return NULL; > } >+ >+ if (!vss_init(true)) { >+ g_debug("vss_init failed, vss commands will not function"); >+ } > #endif > > if (ga_is_frozen(s)) { >-- >2.45.1 > > Otherwise LGTM: Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
On Wed, Jul 03, 2024 at 01:21:29PM +0300, Manos Pitsidianakis wrote: > On Thu, 13 Jun 2024 18:44, "Daniel P. Berrangé" <berrange@redhat.com> wrote: > > The fsfreeze commands are already written to report an error if > > vss_init() fails. Reporting a more specific error message is more > > helpful than a generic "command is disabled" message, which cannot > > beteween an admin config decision and lack of platform support. > > s/beteween/between > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > qga/commands-win32.c | 18 +++--------------- > > qga/main.c | 4 ++++ > > 2 files changed, 7 insertions(+), 15 deletions(-) > > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > > index 2533e4c748..5866cc2e3c 100644 > > --- a/qga/commands-win32.c > > +++ b/qga/commands-win32.c > > @@ -1203,7 +1203,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) > > GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) > > { > > if (!vss_initialized()) { > > - error_setg(errp, QERR_UNSUPPORTED); > > + error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); > > return 0; > > Should this be return -1 by the way? This method has to return GuestFsfreezeStatus and -1 isn't valid. Not a problem though, as the QAPI code will check for *errp != NULL and not consider the return value for error detection. With regards, Daniel
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 2533e4c748..5866cc2e3c 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1203,7 +1203,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp) GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) { if (!vss_initialized()) { - error_setg(errp, QERR_UNSUPPORTED); + error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); return 0; } @@ -1231,7 +1231,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints, Error *local_err = NULL; if (!vss_initialized()) { - error_setg(errp, QERR_UNSUPPORTED); + error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); return 0; } @@ -1266,7 +1266,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp) int i; if (!vss_initialized()) { - error_setg(errp, QERR_UNSUPPORTED); + error_setg(errp, "fsfreeze not possible as VSS failed to initialize"); return 0; } @@ -1961,18 +1961,6 @@ done: /* add unsupported commands to the list of blocked RPCs */ GList *ga_command_init_blockedrpcs(GList *blockedrpcs) { - if (!vss_init(true)) { - g_debug("vss_init failed, vss commands are going to be disabled"); - const char *list[] = { - "guest-get-fsinfo", "guest-fsfreeze-status", - "guest-fsfreeze-freeze", "guest-fsfreeze-thaw", NULL}; - char **p = (char **)list; - - while (*p) { - blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++)); - } - } - return blockedrpcs; } diff --git a/qga/main.c b/qga/main.c index f4d5f15bb3..17b6ce18ac 100644 --- a/qga/main.c +++ b/qga/main.c @@ -1395,6 +1395,10 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation) " '%s': %s", config->state_dir, strerror(errno)); return NULL; } + + if (!vss_init(true)) { + g_debug("vss_init failed, vss commands will not function"); + } #endif if (ga_is_frozen(s)) {
The fsfreeze commands are already written to report an error if vss_init() fails. Reporting a more specific error message is more helpful than a generic "command is disabled" message, which cannot beteween an admin config decision and lack of platform support. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- qga/commands-win32.c | 18 +++--------------- qga/main.c | 4 ++++ 2 files changed, 7 insertions(+), 15 deletions(-)