Re: [libvirt] [PATCH 38/41] remote: use enum helpers for parsing remote driver transport
On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
[...]
+++ b/src/remote/remote_driver.c
+VIR_ENUM_IMPL(remoteDriverTransport,
+ REMOTE_DRIVER_TRANSPORT_LAST,
+ "tls", "unix", "ssh", "libssh2",
"ext", "tcp", "libssh");
One line per enum value, please.
[...]
@@ -174,10 +192,17 @@ static int remoteSplitURIScheme(virURIPtr uri,
+ p = *transport;
+ while (*p) {
+ *p = c_tolower(*p);
+ p++;
+ }
I can't believe we don't have a virString helper for this. Oh well.
[...]
- if (STRCASEEQ(transport_str, "tls")) {
- transport = trans_tls;
[...]
- } else if (STRCASEEQ(transport_str, "libssh"))
{
- transport = trans_libssh;
- } else {
- virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("remote_open: transport in URL not recognised
"
- "(should be
tls|unix|ssh|ext|tcp|libssh2|libssh)"));
+ if ((transport = remoteDriverTransportTypeFromString(transport_str)) <
0)
+ return VIR_DRV_OPEN_ERROR;
You're no longer calling virReportError() when the user attempts to
use an unknown transport. While I don't think hardcoding the list of
valid transport in the error message is a good idea, neither is
failing without telling the user what they did wrong. Please restore
the virReportError() call.
[...]
@@ -966,7 +966,7 @@ doRemoteOpen(virConnectPtr conn,
VIR_DEBUG("Connecting with transport %d", transport);
/* Connect to the remote service. */
switch (transport) {
Idea for a follow-up patch: "transport" could be cast to
remoteDriverTransport here, so that the compiler will ensure we're
covering all possible values.
Anyway, with the VIR_ENUM_IMPL() arguments formatted correctly and
some error message reported on unknown transport,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization