[libvirt] [PATCH 2/6] virNetDevMacVLanTapOpen: Slightly rework

[Laine Stump]

On 12/04/2015 07:30 AM, Michal Privoznik wrote:
> There are few outdated things. Firstly, we don't need to undergo
> the torture of fopen, fscanf and fclose when we have nice wrapper
> over that: virFileReadAll. Secondly, we can use dynamically
> allocated buffer for the interface index.

Nothing against your changes to the existing function (ACK to that), but 
why is it reading sysfs for the ifindex? Why not just use 
virNetDevGetIndex(), as we do everywhere else in libvirt? (For that 
matter, I'm betting that the response message to the netlink request 
that creates the macvtap device will already contain the ifindex of the 
newly created device, but it would take more re-working of the code to 
carry that up from virNetDevMacVLanCreate() and over into 
virNetDevMacVLanTapOpen(), so likely not worth the small efficiency gain).

>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>   src/util/virnetdevmacvlan.c | 29 +++++++++--------------------
>   1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 9384b9f..c1a5f0f 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -237,40 +237,28 @@ int virNetDevMacVLanTapOpen(const char *ifname,
>                               int retries)
>   {
>       int ret = -1;
> -    FILE *file = NULL;
>       char *path;
>       int ifindex;
> -    char tapname[50];
> +    char *tapname = NULL;
> +    char *buf = NULL;
> +    char *c;
>       int tapfd;
>   
>       if (virNetDevSysfsFile(&path, ifname, "ifindex") < 0)
>           return -1;
>   
> -    file = fopen(path, "r");
> -
> -    if (!file) {
> -        virReportSystemError(errno,
> -                             _("cannot open macvtap file %s to determine "
> -                               "interface index"), path);
> +    if (virFileReadAll(path, sizeof(buf), &buf) < 0)
>           goto cleanup;
> -    }
>   
> -    if (fscanf(file, "%d", &ifindex) != 1) {
> +    if (virStrToLong_i(buf, &c, 10, &ifindex) < 0 || *c != '\n') {
>           virReportSystemError(errno,
>                                "%s", _("cannot determine macvtap's tap device "
> -                             "interface index"));
> +                                     "interface index"));
>           goto cleanup;
>       }
>   
> -    VIR_FORCE_FCLOSE(file);
> -
> -    if (snprintf(tapname, sizeof(tapname),
> -                 "/dev/tap%d", ifindex) >= sizeof(tapname)) {
> -        virReportSystemError(errno,
> -                             "%s",
> -                             _("internal buffer for tap device is too small"));
> +    if (virAsprintf(&tapname, "/dev/tap%d", ifindex) < 0)
>           goto cleanup;
> -    }
>   
>       while (1) {
>           /* may need to wait for udev to be done */
> @@ -292,7 +280,8 @@ int virNetDevMacVLanTapOpen(const char *ifname,
>       ret = tapfd;
>    cleanup:
>       VIR_FREE(path);
> -    VIR_FORCE_FCLOSE(file);
> +    VIR_FREE(tapname);
> +    VIR_FREE(buf);
>       return ret;
>   }
>