Hi
On Thu, Sep 29, 2022 at 12:02 PM Alexander Ivanov <
alexander.iva...@virtuozzo.com> wrote:
> In the next patch FreeBSD support for guest-network-get-interfaces will be
> added. Previously move Linux-specific code of HW address getting to a
> separate functions and add a dumb function to commands-bsd.c.
>
> Signed-off-by: Alexander Ivanov <alexander.iva...@virtuozzo.com>
> ---
> qga/commands-bsd.c | 18 +++++++
> qga/commands-common.h | 6 +++
> qga/commands-posix.c | 114 +++++++++++++++++++++++-------------------
> 3 files changed, 87 insertions(+), 51 deletions(-)
>
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index ca06692179..ca81114171 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -167,3 +167,21 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error
> **errp)
> return NULL;
> }
> #endif /* CONFIG_FSFREEZE */
> +
> +#ifdef HAVE_GETIFADDRS
> +/*
> + * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
> + * buffer with ETHER_ADDR_LEN length at least.
> + *
> + * Returns -1 in case of an error, 0 if MAC address can't be obtained or
> + * 1 if MAC addres is obtained.
>
Not a typical Error function return value...
Eventually, you could return a bool for error/ok and take an additional
"bool *obtained/valid" argument. Just a suggestion.
> + */
> +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp)
> +{
> + /*
> + * We can't get the hw addr of this interface,
> + * but that's not a fatal error.
> + */
> + return 0;
> +}
> +#endif /* HAVE_GETIFADDRS */
> diff --git a/qga/commands-common.h b/qga/commands-common.h
> index 2d9878a634..2485a037fd 100644
> --- a/qga/commands-common.h
> +++ b/qga/commands-common.h
> @@ -56,6 +56,12 @@ int64_t qmp_guest_fsfreeze_do_freeze_list(bool
> has_mountpoints,
> int qmp_guest_fsfreeze_do_thaw(Error **errp);
> #endif /* CONFIG_FSFREEZE */
>
> +#ifdef HAVE_GETIFADDRS
> +#include <ifaddrs.h>
> +
> +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp);
> +#endif
> +
> typedef struct GuestFileHandle GuestFileHandle;
>
> GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 6ce894ca6e..0bebd9e690 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -41,20 +41,12 @@
> #endif
> #endif
>
> -#ifdef __FreeBSD__
> -/*
> - * The code under HAVE_GETIFADDRS condition can't be compiled in FreeBSD.
> - * Fix it in one of the following patches.
> - */
> -#undef HAVE_GETIFADDRS
> -#endif
> -
> #ifdef HAVE_GETIFADDRS
> #include <arpa/inet.h>
> #include <sys/socket.h>
> #include <net/if.h>
> +#include <net/ethernet.h>
> #include <sys/types.h>
> -#include <ifaddrs.h>
> #ifdef CONFIG_SOLARIS
> #include <sys/sockio.h>
> #endif
> @@ -2659,14 +2651,6 @@ int64_t
> qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> return -1;
> }
>
> -void qmp_guest_set_user_password(const char *username,
> - const char *password,
> - bool crypted,
> - Error **errp)
> -{
> - error_setg(errp, QERR_UNSUPPORTED);
> -}
> -
>
Why this in this patch?
> GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> {
> error_setg(errp, QERR_UNSUPPORTED);
> @@ -2804,7 +2788,15 @@ out:
> close(datafd[1]);
> }
> }
> -#endif
> +#else /* __linux__ || __FreeBSD__ */
> +void qmp_guest_set_user_password(const char *username,
> + const char *password,
> + bool crypted,
> + Error **errp)
> +{
> + error_setg(errp, QERR_UNSUPPORTED);
> +}
> +#endif /* __linux__ || __FreeBSD__ */
>
> #ifdef HAVE_GETIFADDRS
> static GuestNetworkInterface *
> @@ -2887,6 +2879,54 @@ static int guest_get_network_stats(const char *name,
> return -1;
> }
>
> +#ifndef __FreeBSD__
> +/*
> + * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
> + * buffer with ETHER_ADDR_LEN length at least.
> + * Returns -1 in case of an error, 0 if MAC address can't be obtained or
> + * 1 if MAC addres is obtained.
> + */
> +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp)
> +{
> + struct ifreq ifr;
> + int sock;
> +
> + /* we haven't obtained HW address yet */
> + sock = socket(PF_INET, SOCK_STREAM, 0);
> + if (sock == -1) {
> + error_setg_errno(errp, errno, "failed to create socket");
> + return -1;
> + }
> +
> + memset(&ifr, 0, sizeof(ifr));
> + pstrcpy(ifr.ifr_name, IF_NAMESIZE, ifa->ifa_name);
> + if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> + /*
> + * We can't get the hw addr of this interface, but that's not a
> + * fatal error. Don't set info->hardware_address, but keep
> + * going.
> + */
> + if (errno == EADDRNOTAVAIL) {
> + /* The interface doesn't have a hw addr (e.g. loopback). */
> + g_debug("failed to get MAC address of %s: %s",
> + ifa->ifa_name, strerror(errno));
> + } else{
> + g_warning("failed to get MAC address of %s: %s",
> + ifa->ifa_name, strerror(errno));
> + }
> + close(sock);
> + return 0;
> + }
> +#ifdef CONFIG_SOLARIS
> + memcpy(buf, &ifr.ifr_addr.sa_data, ETHER_ADDR_LEN);
> +#else
> + memcpy(buf, &ifr.ifr_hwaddr.sa_data, ETHER_ADDR_LEN);
> +#endif
> + close(sock);
> + return 1;
> +}
> +#endif /* __FreeBSD__ */
> +
> /*
> * Build information about guest interfaces
> */
> @@ -2907,10 +2947,9 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
> GuestNetworkInterfaceStat *interface_stat = NULL;
> char addr4[INET_ADDRSTRLEN];
> char addr6[INET6_ADDRSTRLEN];
> - int sock;
> - struct ifreq ifr;
> - unsigned char *mac_addr;
> + unsigned char mac_addr[ETHER_ADDR_LEN];
> void *p;
> + int ret;
>
> g_debug("Processing %s interface", ifa->ifa_name);
>
> @@ -2924,45 +2963,18 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
> }
>
> if (!info->has_hardware_address) {
> - /* we haven't obtained HW address yet */
> - sock = socket(PF_INET, SOCK_STREAM, 0);
> - if (sock == -1) {
> - error_setg_errno(errp, errno, "failed to create socket");
> + ret = guest_get_hw_addr(ifa, mac_addr, errp);
> + if (ret == -1) {
> goto error;
> }
> -
> - memset(&ifr, 0, sizeof(ifr));
> - pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->name);
> - if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> - /*
> - * We can't get the hw addr of this interface, but that's
> not a
> - * fatal error. Don't set info->hardware_address, but keep
> - * going.
> - */
> - if (errno == EADDRNOTAVAIL) {
> - /* The interface doesn't have a hw addr (e.g.
> loopback). */
> - g_debug("failed to get MAC address of %s: %s",
> - ifa->ifa_name, strerror(errno));
> - } else{
> - g_warning("failed to get MAC address of %s: %s",
> - ifa->ifa_name, strerror(errno));
> - }
> -
> - } else {
> -#ifdef CONFIG_SOLARIS
> - mac_addr = (unsigned char *) &ifr.ifr_addr.sa_data;
> -#else
> - mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
> -#endif
> + if (ret == 1) {
> info->hardware_address =
> g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
> (int) mac_addr[0], (int) mac_addr[1],
> (int) mac_addr[2], (int) mac_addr[3],
> (int) mac_addr[4], (int) mac_addr[5]);
> -
> info->has_hardware_address = true;
> }
> - close(sock);
> }
>
> if (ifa->ifa_addr &&
> --
> 2.34.1
>
>
>
looks ok to me otherwise
--
Marc-André Lureau
