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@xxxxxxxxxx>
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
