On Tue, Feb 27, 2024 at 6:32 PM Anthony PERARD <anthony.per...@cloud.com> wrote:
>
> On Tue, Feb 27, 2024 at 01:26:28PM +0000, Fouad Hilly wrote:
> > diff --git a/tools/libs/stat/xenstat_linux.c
> > b/tools/libs/stat/xenstat_linux.c
> > index cbba54aa83ee..6d82e204aad4 100644
> > --- a/tools/libs/stat/xenstat_linux.c
> > +++ b/tools/libs/stat/xenstat_linux.c
> > @@ -390,6 +390,38 @@ void xenstat_uninit_networks(xenstat_handle * handle)
> > fclose(priv->procnetdev);
> > }
> >
> > +static int read_attributes_vbd3(char *vbd3_path, xenstat_vbd *vbd)
>
> "const char *vbd3_path"?
>
>
> > static int read_attributes_vbd(const char *vbd_directory, const char
> > *what, char *ret, int cap)
> > {
> > static char file_name[80];
> > @@ -438,7 +470,7 @@ int xenstat_collect_vbds(xenstat_node * node)
> > int ret;
> > char buf[256];
> >
> > - ret = sscanf(dp->d_name, "%3s-%u-%u", buf, &domid, &vbd.dev);
> > + ret = sscanf(dp->d_name, "%255[^-]-%u-%u", buf, &domid,
> > &vbd.dev);
>
> 255 is overly ambitious, but it match the size of buf, so I guess it's
> kind of ok, even if unnecessary.
>
> > if (ret != 3)
> > continue;
> > if (!(strstr(buf, "vbd")) && !(strstr(buf, "tap")))
> > @@ -448,6 +480,8 @@ int xenstat_collect_vbds(xenstat_node * node)
> > vbd.back_type = 1;
> > else if (strcmp(buf,"tap") == 0)
> > vbd.back_type = 2;
> > + else if (strcmp(buf,"vbd3") == 0)
> > + vbd.back_type = 3;
>
> Yay for magic number... Do you think you could introduce an enum or
> define to replace this "3" by a meaningful? Maybe something like
> XENSTAT_VBD_TYPE_VBD3, (name derived from the existing function
> xenstat_vbd_type()).
>
> I'd like at least to replace the "3". But if you feel like having
> another patch to replace the "2" and "1", that would be a plus.
>
> > else
> > vbd.back_type = 0;
> >
> > @@ -479,6 +513,35 @@ int xenstat_collect_vbds(xenstat_node * node)
> > vbd.error = 1;
> > }
> > }
> > + else if (vbd.back_type == 3)
> > + {
> > + char *td3_pid;
> > + char *path;
> > +
> > + vbd.back_type = 3;
>
> `back_type` should already be 3 ;-).
>
> > + vbd.error = 0;
> > +
> > + if (asprintf(&path,
> > "/local/domain/0/backend/vbd3/%u/%u/kthread-pid", domid, vbd.dev) < 0)
> > + continue;
> > +
> > + td3_pid = xs_read(node->handle->xshandle, XBT_NULL,
> > path, NULL);
> > +
> > + free(path);
> > +
> > + if (td3_pid == NULL)
> > + continue;
> > +
> > + if (asprintf(&path, "/dev/shm/td3-%s/vbd-%u-%u",
> > td3_pid, domid, vbd.dev) < 0) {
> > + free(td3_pid);
> > + continue;
> > + }
> > +
> > + if (read_attributes_vbd3(path, &vbd) < 0)
> > + vbd.error = 1;
>
> Why sometime we do "continue" and sometime we do "vbd.error=1"?
continue is used when path to domain statistics is checked, which is
"dynamic"; However, if the path existed but failed to read statistics,
that is when we set the error.
>
> > +
> > + free(td3_pid);
> > + free(path);
> > + }
> > else
> > {
> > vbd.error = 1;
> > diff --git a/tools/libs/stat/xenstat_priv.h b/tools/libs/stat/xenstat_priv.h
> > index 4eb44a8ebb84..c3a9635240e9 100644
> > --- a/tools/libs/stat/xenstat_priv.h
> > +++ b/tools/libs/stat/xenstat_priv.h
> > @@ -98,6 +98,22 @@ struct xenstat_vbd {
> > unsigned long long wr_sects;
> > };
> >
> > +struct vbd3_stats {
> > + uint32_t version;
> > + uint32_t __pad;
> > + uint64_t oo_reqs;
> > + uint64_t read_reqs_submitted;
> > + uint64_t read_reqs_completed;
> > + uint64_t read_sectors;
> > + uint64_t read_total_ticks;
> > + uint64_t write_reqs_submitted;
> > + uint64_t write_reqs_completed;
> > + uint64_t write_sectors;
> > + uint64_t write_total_ticks;
> > + uint64_t io_errors;
> > + uint64_t flags;
> > +};
>
> Is that a binary interface for an external project? I'm asking because
> `__pad` would seems useless otherwise.
> If it is part of an interface, please add a comment about it, add a link
> to the project/source where this interface is described, and where is
> the canonical location? Also, is there an header we could maybe just
> use if it's in the system or an header we could import into the
> repository?
>
> Thanks,
>
> --
> Anthony PERARD
